-
Notifications
You must be signed in to change notification settings - Fork 324
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
[SPIKE] Refactor checkboxes and radios #4040
Conversation
Uh oh! @owenatgov, the image you shared is missing helpful alt text. Check your pull request body. Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
This is very cool, but I'm kinda surprised it works. There's a bug raised against Chromium to fix this 'incorrect' behaviour, which references an interesting discussion about deprecating it – ultimately it seems they decided not to because it would break too many sites. There are a few references to allowing inputs with So, generally this solution feels 'neater' than the current solution, but is at best in a grey area when we look at validating it against the spec. Not against exploring it further, but we need to bear this in mind and make sure we're comfortable with the risk. We'd also need to check how recently support for Footnotes
|
I've used this technique for a few years, so in my experience it seems robust and well supported on all the modern user agents, intentionally or not. My best guess as to why it works is that browsers are resetting some of the element's styling 'rules' in response to This would seem to be supported by the pseudo-elements being completely ignored if you remove You could argue that the CSS spec is being a bit vague here:
As |
Thanks both. This is insightful stuff. So, I personally don't feel confident introducing the risk of us going against the spec if we can avoid it, even if the problem space is quite wooly right now. It's a shame because it'd make this work a lot easier so I'm going to do some exploration on the "if we can avoid it" part. @querkmachine I haven't been able to replicate removing An immediate thought which comes to mind is to have a wrapper around the input itself to store the psuedo elements but I'd like to avoid this if we can as we're already introducing extra markup to get around issues with positioning of hints and if we're introducing a wrapper, we might as well serve the styling from the I'm going to investigate how well the flexbox solution works if we restrict it to the current setup of the visual input coming from the |
Hmm, seems like that's only true in Firefox actually. I might be misremembering from past experience where I shoved a bunch of the styles behind a |
To verify that above comment, I tried removing |
I did a bit of experimenting and have managed to make the current positioning solution work if the pseudo elements are moved to either the label or the container. This would mean that all we're really doing is swapping out the input handling the touch target for it instead handling the visual container whilst pseudo elements do the rest, the same as how things are now. This to me defeats the purpose of the refactor as the intent of change 2 was to try and make the pieces of the interactive input (touch target, outline, check indicator) relate to the input element. At the same time, we now know we can comfortably achieve change 1 without the need for change 2. I've additionally explored trying to handle the styling of the check indicator solely within the input element, no pseudo elements. This is basically impossible for checkboxes due to the complexity of the shape and whilst radios are feasible using tricks like inset box shadows or I'm going to briefly revisit if we actually need the new container element or if there's a way to not interfere with hint positioning. Open to extra thoughts. |
d8e6bbe
to
b5c0f47
Compare
I've managed to remove the container 🎉 What I did:
This means that this is no longer a breaking change. If folks are happy with this implementation, I think we're more or less in a position to production-ify this (including rolling back the refactoring of the input elements) and merge it. |
I'm now going to close this in favour of it coming at it fresh and with code sanitation in mind. There aren't any objections to the solution used so I'm going to apply the positioning changes in this spike. @36degrees didn't get a chance to comment before going on leave but one thing I'll keep in mind that he mentioned to me in person last week is to not use |
What
Makes the following changes to checkboxes and radios:
input
itself viaappearance: none
as opposed to using pseudo elements.Why
Change 1 is tied to #3898. Making the spacing more dynamic means that we avoid exacerbating the existing very subtle alignment issue on small radios and checkboxes as well as future proofing us from other architectural changes like the typography scale.
Change 2 manifested whilst trying to figure out change 1 and is the more radical change. The use case for this is that the visual construction of the elements "make more sense" ie: what the user is interacting with visually is the input itself as opposed to pseudo elements with the input operating behind the visual elements aka: it feels less hacky.
Visual changes
The visual changes here are intended to be minimal aside from the existing alignment issue. It is most prominent on the small variants of checkboxes and radios:
Before
After
Additional notes
How checkboxes and radios fit together now
The visual elements within radios and checkboxes have moved around like so:
before
of thelabel
to be theinput
itselfafter
of thelabel
to theafter
of theinput
before
of theinput
now handles the touch target, which the input was previously doingThe position of the
input
and thelabel
and elements within thelabel
, including text, are handed by flexbox.Theinput
andlabel
positioning is specifically handled by a new container element:govuk-[checkboxes/radios]__container
.The addition of a new element in the macro unfortunately means that this is a breaking changeDifferences in older browsers
Because this change uses
appearance
, a feature with no support on IE and below, this is potentially a significant visual regression for IE users. This isn't as much of an issue as we will only be supporting IE "functionally" from 5.0, however here is what these changes look like in versions of IE compared to how they look currently:Code sanitation
Because this is an impromptu spike, I haven't focused as much on keeping the code neat. Therefore there are some comments in odd places that I haven't deleted and some things that probably should have comments that don't. If anything doesn't make sense I can try to explain. Additionally, I'm not looking for advice on code formatting as much as I am on the implementation choices I've made.