-
Notifications
You must be signed in to change notification settings - Fork 329
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
Refactor positioning of radios and checkboxes #4093
Conversation
The small radios and checkboxes have changed in that the input is no longer left-aligned with the label:
Looks like this hasn't been caught by Percy either – worth adding small radios and checkboxes, and maybe examples with hints, to our list of extra visual regression examples? Could be worth adding them with a separate PR to main and then rebasing so we can see the diff here. |
74cb3da
to
ddfa48d
Compare
@36degrees Good idea, I'll make a separate PR for this. I've addressed this comment in the mean time. |
Percy PR spun off from this one #4108 |
ddfa48d
to
8f5d781
Compare
@@ -4,23 +4,18 @@ | |||
@import "../label/index"; | |||
|
|||
@include govuk-exports("govuk/component/checkboxes") { | |||
$govuk-touch-target-size: 44px; | |||
$govuk-touch-target-gutter: 4px; |
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 PR looks brill. Thanks for clarifying the gutter
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/checkboxes/_index.scss
Outdated
Show resolved
Hide resolved
8f5d781
to
b83975f
Compare
This one's got some (good) conflicts to de-duplicate If you don't mind doing another rebase please 🙏 |
b83975f
to
143538b
Compare
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 2486fed |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
143538b
to
64d6799
Compare
64d6799
to
4f60494
Compare
Are you happy with the Percy diff @owenatgov? The gutter changes look good, but did we mean to intentionally space things out vertically? |
Now also applied to radios 👀 |
@owenatgov Bit annoying, cheers for that Hmm, might have to tackle it though as it's made checkboxes (without hints) really close together 😣 |
🤦🏻 🤦🏻 🤦🏻 Yeah that'll do that won't it... Let's have another go. |
f4c4316
to
3d56e10
Compare
3d56e10
to
93e0a93
Compare
@colinrotherham I cracked it by just replicating the hint's specificity. Probably the best solution for this for the moment, unless you can figure out how to collapse margins in flexbox (I couldn't!). |
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.
Thanks @owenatgov, looking good
Can you double check small radios/checkboxes for me?
Percy is showing wider spacing everywhere (good) but for small ones only the labels have shifted a tiny bit closer to their inputs. If design people are happy, go for it
This means that the label content is still centred against the input whilst not interfering with inline/block element dynamics within the label
93e0a93
to
2486fed
Compare
@colinrotherham I think I've figured it out. On the live version, small variants have a 1px right padding. I couldn't find any concrete reasoning from a quick history skim but my suspicion is that it's to create a little bit of space between the small hover state and the label. For ease I've reintroduced it. Whatcha reckon? |
…actor Refactor positioning of radios and checkboxes
This was raised by the renovate bot to begin with but was causing the integration tests to fail with the error `is being covered by another element`, because it couldn't check radio and checkboxes. Looking at what is in the 5.1.0 release, it includes a change to radio and checkbox styling, which I believe is what is causing the issue. alphagov/govuk-frontend#4093. It looks like the removal of the `z-index` on `.govuk-radios__input` and `.govuk-checkboxes__input` has meant they now have `z-index: auto` but the styled `label:before` and :`after` have `z-index: 1` which causes them to be higher in the `z-order` than the inputs, which causes the cypress error above. Updating `selectRadioButton()` and adding `selectCheckbox()` to `check()` with `force: true`, forces the click on the radio button even though it is covered by the labels pseudo elements.
Looking at what is in the 5.1.0 release, it includes a change to radio and checkbox styling, which I believe is what is causing the issue. alphagov/govuk-frontend#4093. It looks like the removal of the `z-index` on `.govuk-radios__input` and `.govuk-checkboxes__input` has meant they now have `z-index: auto` but the styled `label:before` and :`after` have `z-index: 1` which causes them to be higher in the `z-order` than the inputs, which means they’re underneath another element and technically not clickable. Adding `force: true`, forces the click on the radio button and checkbox even though it is covered by the labels pseudo elements. This does not affect the actual usability.
Force Cypress to check checkboxes if it thinks they're overlapped. Looks to be caused by changes in GOV.UK Frontend v5.1.0 (alphagov/govuk-frontend#4093)
What
Refactor radios and checkboxes to use flexbox to handle positioning. Specific changes:
__item
's are usingflex-wrap: wrap
Why
To make the positioning of radios and checkboxes dynamic in preparation for the new typography scale.
Closes #3898
Based off work done in #4040
Notes
Since this is only a refactor of the underlying positioning of the components, I'm expecting the Percy diff to be next to nothing, no more than a few pixels.
Whilst this is a significant change for us, it is intended to make little to no difference to users and therefore should be treated as a bug fix change.