-
Notifications
You must be signed in to change notification settings - Fork 334
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 selecting options by passing current values #2616
Conversation
CHANGELOG.md
Outdated
|
||
This change was introduced in [pull request #2616: Allow selecting options by passing current values](https://github.com/alphagov/govuk-frontend/pull/2616). | ||
|
||
#### Select an option in a Select by using the `selectedValue` option |
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 A NIGHTMARE. Two of these words have two different meanings and we use both meanings in the same heading.
Select (verb) an option (an <option>
HTML element within a <select>
) in a Select (component name / HTML element name) using the selectedValue
option (a 'Nunjucks macro option')
Suggestions welcome.
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.
Tricky one alright! Ok, first pass:
Choose an option in a select component by using the
selectedValue
parameter
Who is it that does the 'choosing' or 'selecting' of the option? Is the idea here that the service pre-selects something to save the end user time?
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 might be to save the user time, or more likely it might be because the user has gone back to change an answer they previously gave.
For example, if a user was in the middle of a transaction and went 'back' to a previous page that used a select, the answer they previously gave should be selected when the page loads.
Likewise, if the user was checking their answers and decided to change an answer they previously gave, when they click change the page that loads should pre-select the answer that they'd previously given.
@EoinShaughnessy the changelog entry for this PR is a bit gnarly – could do with your thoughts on it when you have a mo. There's no rush on it though, I don't think we should try and get this into v4.1.0 so it can wait for the next release. |
I've raised a PR with a proof of concept of how this could be extended in the Prototype Kit to allow pre-selecting radios and checkboxes from the |
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.
Left a few small comments. Content looks good though! Separating it out was a neat idea.
I'd argue strongly for calling it |
Interesting! I understand your argument but I'd argue that using We also have a principle that says we should mimic HTML attribute names and although it's not explicitly stated there I think it also makes sense to avoid using common HTML attribute names when the option doesn't translate directly to that attribute on any of the elements within the component. (Having said that… we already use Fundamentally… I'm wondering whether we're better off trying to 'abstract away' some of the messiness and inconsistencies in the HTML spec, or to try to expose it and be consistent with it? 🤔 Using
|
7d78d62
to
c23444f
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.
Nice work! This is a bit similar to a filter we use to make the macros simpler.
Note you probably want the ability for a single item to have checked: false
- at the moment the logic will overwrite these. Basically - if a single item has a value for checked
or selected
you should honour that and not override it.
This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected.
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
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.
Looking really nice! I've left a couple of comments about tech docs. The solution itself is very slick 👍🏻
@@ -69,7 +69,7 @@ params: | |||
- name: checked | |||
type: boolean | |||
required: false | |||
description: If `true`, radio will be checked. | |||
description: If `true`, the radio will be checked when the page loads. |
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.
Same comment on communicating the override
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.
description: If `true`, the radio will be checked when the page loads. | |
description: Whether the radio should be checked when the page loads. If you set this option, the `value` option will not affect the checked state of the radio. |
@@ -23,7 +23,7 @@ params: | |||
- name: selected | |||
type: boolean | |||
required: false | |||
description: Sets the option as the selected. | |||
description: Sets the option as selected when the page loads. |
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.
Same comment on communicating the override
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
At the minute the only way to pre-select or check specific items when using the Nunjucks macros for these components is to pass a boolean flag for each item, which then maps directly to the checked / selected HTML attribute for the <input> / <option>. This means the user has to do the work to map the previous form data back to a boolean, resulting in code that might look something like this: ``` {{ govukSelect({ id: "sort", name: "sort", label: { text: "Sort by" }, items: [ { value: "published", text: "Recently published", selected: (data['sort'] == "published") }, { value: "updated", text: "Recently updated", selected: (data['sort'] == "updated") }, { value: "views", text: "Most views", selected: (data['sort'] == "views") }, { value: "comments", text: "Most comments", selected: (data['sort'] == "comments") } ] }) }} ``` This is prone to introducing errors (especially when prototyping) – for example, any changes to the name of the field or the value of the item also need corresponding changes made to the boolean logic, potentially in multiple places. Allow the user to pass the current value (or array of values, for checkboxes), and using that to work out which item(s) should be checked or selected. The above example can then be done like this: ``` {{ govukSelect({ id: "sort", name: "sort", label: { text: "Sort by" }, items: [ { value: "published", text: "Recently published" }, { value: "updated", text: "Recently updated" }, { value: "views", text: "Most views" }, { value: "comments", text: "Most comments" } ], value: data[‘sort’] }) }} ```
Co-authored-by: EoinShaughnessy <72507742+EoinShaughnessy@users.noreply.github.com>
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
I think I've addressed the comments from @owenatgov but it could do with another once over. Then I believe this is ready to merge once v4.1.1 is out. |
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.
Looking great 👍🏻
We've now decided to skip v4.1.1 and roll everything into v4.2.0, so this can be merged. |
If a checkbox, radio or select option has the checked or selected option set, always use its value to set the checked or selected state, even when it’s falsey. As suggested by @edwardhorsford [1]: > This is helpful when most of the items just need to check if the stored value equals the value, but one or two need more complex logic to determine if they should be selected. [1]: #2616 (review)
At the minute the only way to pre-select or check specific items when using the Nunjucks macros for these components is to pass a boolean flag for each item, which then maps directly to the checked / selected HTML attribute for the / .
This means the user has to do the work to map the previous form data back to a boolean, resulting in code that might look something like this:
This is prone to introducing errors (especially when prototyping) – for example, any changes to the name of the field or the value of the item also need corresponding changes made to the boolean logic, potentially in multiple places.
Allow the user to pass the current value (or array of values, for checkboxes), and using that to work out which item(s) should be checked or selected.
The above example can then be done like this:
Naming the new option has presented a few challenges – I wanted to avoid usingchecked
orselected
so that we don’t have options at different ‘levels’ with the same name but accepting different types of values – booleans for individual items and strings / arrays at the top level.I also felt thatvalue
was not an accurate description for what the option did, and may also be confused withitem.value
(and the top-levelvalue
option on other form controls such as text inputs).I’ve ended up combining the two whilst also trying to be consistent with the underlying HTML attribute, and also reflecting whether the option accepts a string or an array. This does unfortunately mean that the option name is different for each of the three components:checkedValue
for Radios, where a string is passed to check a single radio buttoncheckedValues
for Checkboxes, where an array of strings is passed to check one or more checkboxesselectedValue
for the Select, where a string is passed to select a single option.I think on balance, although I can see how this may be confusing for users, it is logical and consistent within each individual component.After further discussion within the team off the back of #2616 (comment) we ended up going with
value
/values
.Closes #2449.