-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow for an optional divider between radio items #849
Conversation
src/components/radios/radios.yaml
Outdated
text: Use Government Gateway | ||
- value: govuk-verify | ||
text: 'Use GOV.UK Verify' | ||
- divider: 'or' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could follow the same rules as with all our text params and allow for either text or html, but surely it should only be text. it could be
- divider: {
text: text here
}
as well
7f71a88
to
c71006e
Compare
@dashouse question about text alignment in relationship to the radio. in elements it's aligned to the left |
Design wise 👍 |
src/components/radios/_radios.scss
Outdated
@include govuk-font($size: 19); | ||
display: block; | ||
margin-bottom: govuk-spacing(2); | ||
padding: 0 0 0 ($govuk-radios-size / 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is visually centring the text, but it's relying on the string being 'or' - if you use a different string then it doesn't work. What about making this element the same width as the radio buttons / checkboxes and then use text-align: center
? That way it should work correctly for short strings, and automatically fall back to left-aligning if the string is long enough that centring it doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we agreed to go with Ollie's suggestions
src/components/radios/radios.yaml
Outdated
name: example | ||
fieldset: | ||
legend: | ||
text: Have you changed your name? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this example make sense? The answers don't relate to the question.
src/components/radios/template.njk
Outdated
@@ -44,9 +44,12 @@ | |||
<div class="govuk-radios {%- if params.classes %} {{ params.classes }}{% endif %}" | |||
{%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %} | |||
{%- if isConditional %} data-module="radios"{% endif -%}> | |||
{% for item in params.items %} | |||
{% for item in params.items %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace
src/components/radios/template.njk
Outdated
@@ -59,12 +62,13 @@ | |||
attributes: item.label.attributes, | |||
for: id | |||
}) | indent(6) | trim }} | |||
</div> | |||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace
c71006e
to
36c5e5b
Compare
36c5e5b
to
36d2706
Compare
36d2706
to
f01a6e1
Compare
f01a6e1
to
d29e0b0
Compare
d29e0b0
to
714bfda
Compare
714bfda
to
d561e25
Compare
d561e25
to
dff8aec
Compare
dff8aec
to
9671ffe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks really good, but I think the argument docs could be clearer.
src/components/radios/index.njk
Outdated
text: 'No' | ||
}, | ||
{ | ||
html: 'Optional divider to separate options, e.g "or".' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to be clearer how to use this – it's not obvious for example that specifying 'divider' means that all other options will be ignored.
9671ffe
to
21991cd
Compare
21991cd
to
d3c0ee0
Compare
src/components/radios/template.njk
Outdated
@@ -47,6 +47,9 @@ | |||
{% for item in params.items %} | |||
{% set id = item.id if item.id else idPrefix + "-" + loop.index %} | |||
{% set conditionalId = "conditional-" + id %} | |||
{%- if item.divider %} | |||
<span class="govuk-radios__divider">{{ item.divider }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be a div
, instead of span
that has display: block
set. It would make it consistent with the markup of the radios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's now a div
d3c0ee0
to
3813723
Compare
src/components/radios/_radios.scss
Outdated
.govuk-radios__divider { | ||
$govuk-divider-size: $govuk-radios-size !default; | ||
@include govuk-font($size: 19); | ||
display: block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant now that the divider is a div
.
3813723
to
31a21b6
Compare
31a21b6
to
5bb1129
Compare
@@ -138,6 +138,14 @@ | |||
} | |||
} | |||
|
|||
.govuk-radios__divider { | |||
$govuk-divider-size: $govuk-radios-size !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work well but I wonder if it'll stand up to the following scenarios:
- A divider longer than 'or'
- A user changing their text size
It may be more flexible to have this span the full width of the component, and align it with the radios by some padding-left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the original approach – see related discussion in #849 (comment)
When the divider is longer than or the text will overflow to the right, like this:
I don't think we'd tested with users changing their text size though – good call. It seems to work though, having enabled $govuk-typography-use-rem: true;
in app.scss:
Firefox, with size set to 28 in appearance settings:
Safari, with 'Never use font sizes smaller than 24' configured under 'Advanced':
Chrome, with 'Font size' set to very large in appearance settings:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet thanks for that. 👍
5bb1129
to
f161ac3
Compare
f161ac3
to
7b03bee
Compare
7b03bee
to
6b2b82a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really well thought out. Great naming for the API choices 👍
6b2b82a
to
95b8ffa
Compare
It used to be a feature in GOV.UK Elements and it's not currently available in our macros.
Direct url: https://govuk-frontend-review-pr-849.herokuapp.com/components/radios/with-a-divider/preview
This PR:
Fixes: #729
Works fine with conditional reveal as well.
Screenshots
IE 8
IE 9
IE 11
Firefox 60
Safari 9.3