Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes for a couple of component examples / fixtures #2043

Merged
merged 5 commits into from
Dec 15, 2020
Merged

Fixes for a couple of component examples / fixtures #2043

merged 5 commits into from
Dec 15, 2020

Conversation

andymantell
Copy link
Contributor

I have spotted a couple of issues with the current fixtures whilst using them in govuk-react-jsx. This PR is to fix them...

  1. on the fieldset params example in the checkboxes component, the legend param was not nested underneath the fieldset param.

  2. On the select component, value is not marked as required underneath the items / individual options. If not supplied, the component will render every option with an empty value which I think doesn't make much sense. I have therefore marked it as required, and fixed two of the hidden examples to match (All the other examples already had value specified. This was causing problems in my react port since React cannot have two <option> elements with the same underlying value, even if that value is blank.

@andymantell
Fix invalid legend on the checkbox component's 'fieldset params' example
6deb199
@andymantell
Mark 'value' as required under each option of a select component
d824b00 
Omitting this value causes the select to output empty values
against each item and so it should be marked as required.

Also fixed a couple of the examples to comply with the above.
@andymantell
Copy link
Contributor Author

Ah, I spy a failing snapshot, will sort after lunch

andymantell added a commit to surevine/govuk-react-jsx that referenced this pull request Nov 26, 2020
@andymantell
Skip 'checkboxes:fieldset params' test until alphagov/govuk-frontend#…
b6ad4d2 
…2043 is released
@andymantell andymantell marked this pull request as draft November 26, 2020 14:23
@andymantell
Copy link
Contributor Author

I've marked this as a draft for a second as I've got more changes coming...

@andymantell andymantell marked this pull request as ready for review November 26, 2020 16:26
@andymantell
Copy link
Contributor Author

That should be all the changes now so this is ready for review. I've got govuk-react-jsx passing 100% against the govuk-frontend fixtures now, with the changes made in this pull request hacked into my library as overrides.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-component--q45mbj November 30, 2020 13:43 Inactive
@36degrees 36degrees added this to the [NEXT] milestone Dec 14, 2020
@vanitabarrett
Copy link
Contributor

Thank you for raising this PR @andymantell ! I think on the whole we're happy with these changes and it's great to see the examples being made more consistent!

However, we're not sure about marking value as a required option for selects. At the moment, not providing a value means you end up with an empty string as a value - this is valid HTML. It's possible that in the future we may want to revisit the macro behaviour so that not passing value actually results in what you would perhaps expect (no value on the option), and therefore marking it as required would be correct. But this would be a breaking change, so not something we're in a position to look at yet.

Would you be happy with updating this PR to remove the change that marks value as required? If so, I'll also ask our tech writer to take a look to see if we can update the documentation for that option to make it clearer that you get an empty string if not provided.

@andymantell
Copy link
Contributor Author

Ok that's fair enough. I'll remove the required option. I suppose even though I would consider that output with duplicate empty values to be somewhat weird, there may be some use case that I haven't envisaged...

From my perspective, so long as the actual examples contain values that are unique and non empty then that works.

@andymantell
Unmark select component option values as required
beb0e6d 
Followup to d824b00 and feedback
on corresponding pull request
(alphagov#2043)
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-fronte-component--q45mbj December 14, 2020 16:58 Inactive
@andymantell
Copy link
Contributor Author

@vanitabarrett That's done now

vanitabarrett pushed a commit that referenced this pull request Dec 15, 2020
Clarify default value for 'value' option in select component
57bd522 
Prompted by #2043, this commit adds a line to the docs for the value option to explain that it defaults to an empty string.
@vanitabarrett vanitabarrett mentioned this pull request Dec 15, 2020
Merged
36degrees
36degrees approved these changes Dec 15, 2020
View reviewed changes
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andymantell 🙏🏻

@36degrees 36degrees merged commit e3c4b81 into alphagov:master Dec 15, 2020
@andymantell andymantell deleted the component-examples-tweaks branch December 15, 2020 14:48
@36degrees 36degrees mentioned this pull request Dec 17, 2020
Merged
This was referenced Mar 16, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vanitabarrett vanitabarrett vanitabarrett approved these changes

@36degrees 36degrees 36degrees approved these changes

Assignees
No one assigned
Labels
None yet
Projects
GOV.UK Design System cycle board
Archived in project
Milestone
v3.10.2
Development

Successfully merging this pull request may close these issues.
4 participants
@andymantell @vanitabarrett @36degrees @govuk-design-system-ci