-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update search component #2462
Update search component #2462
Conversation
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.
A comment and a question. Very nice work here, some lovely tests.
@@ -32,11 +28,31 @@ | |||
classes << "gem-c-search--on-white" | |||
end | |||
classes << "gem-c-search--separate-label" if local_assigns.include?(:inline_label) or local_assigns.include?(:label_size) |
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.
A teeny tiny nitpick: you've mentioned why it's a bad idea to specify the inline_label
using the ||=
convention at the top (because it's falsey, ruby will defer to the default value). Do you think its worth adding a comment to this effect? I'm thinking of a well-meaning dev seeing that it's not included at the top and adding it in, potentially breaking a few things.
I'm asking instead of suggesting partly because the tests would catch this and partly to document this quirk of ruby in our PR history.
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.
Ooh, this is a can of worms. I agree, it's a thing that might be overridden without someone meaning to - so we could suggest moving away from using false
values as the default.
For this component, I'd also suggest that renaming the inline_label
to label_position
of which the options are inline
or above
(or something similar, just throwing ideas out here). Then we can set the default to be inline
using the ||=
, run the two parameters in parallel while we update the component usage, and then deprecate it. I think there are a couple of breaking changes coming up, which is good timing.
I'll raise an issue to gently nudge a discussion around this and see what the community consensus is.
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.
Issue raised: #2472
Approved the percy build 👍 |
All the uses of `[class='^=class-name']` have been replaced with the correct formatting: `[class^='class-name']`. As highlighted by @owenatgov in another review[1], there were selectors in the spec that weren't formatted correctly - so they were potentially able to giving false negatives. [1]: #2462 (comment)
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 should've mentioned this in my original review but this just needs a changelog update and I'll approve it :)
ba38459
to
3c495f8
Compare
@owenatgov change log updated - ready for 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.
🔍
What
margin_bottom
parameter - only the documentation was added here, not the functionality.isPageHeading
setting in GOV.UK Frontend's Nunjucks macro. This is best used in conjunction withlabel_size
.Why
The documentation for the
margin_bottom
attribute was missing, so needed to be added.The spacing between the label and the input needed to be more flexible to allow for the updated homepage layout.
Having the label wrapped as a heading will allow for a the structure of the page to make semantic sense without needing unnecessary repetition - for example, a heading saying "Search" followed by a label that says "Search GOV.UK" could be merged into one heading-label that says "Search GOV.UK".
Visual Changes
Example with
label_margin_bottom
set to 9:There are no visual changes when using
wrap_label_in_a_heading
- this will wrap the label in a heading element that is unstyled with the margin removed.