Update search component #2462
Update search component #2462
Conversation
| @@ -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.
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.
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.
|
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)
injms
force-pushed
the
update-search-component
branch
from
ba38459
to
3c495f8
Compare
|
@owenatgov change log updated - ready for review |
What
margin_bottomparameter - only the documentation was added here, not the functionality.isPageHeadingsetting in GOV.UK Frontend's Nunjucks macro. This is best used in conjunction withlabel_size.Why
The documentation for the
margin_bottomattribute 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_bottomset 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.