-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Header] No-js behaviour for locale selectors #2485
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.
LGTM. It really makes the header big, but it works!
<noscript class="small-hide medium-hide"> | ||
{%- form 'localization', id: 'HeaderCountryMobileFormNoScript', class: 'localization-form' -%} | ||
<div class="localization-form__select"> | ||
<h2 class="visually-hidden" id="HeaderCountryMobileLabelNoScript">{{ 'localization.country_label' | t }}</h2> |
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.
Why h2
and not a label
?
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.
Reusing the approach from the footer and also what's in the JS approach. I believe the reason why is because it helps navigate to it more easily when using a screen reader and using the VoiceOver rotor which shows you all the page's headings.
{%- form 'localization', id: 'HeaderCountryForm', class: 'localization-form' -%} | ||
<div class="no-js-hidden"> |
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.
Non blocking but just curious: do you know why no-js-hidden
was on the inner div before?
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'm not sure tbh. If I left it the way it was, I'd get some unwanted space around the selectors. It's like that in the footer, so I think it was mostly copied over.
I can't really think of a reason why it wouldn't be on the parent 🤔
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.
The arrow buttons inside of the selector container are not clickable. Although UX in general isn't great without JS this small issue might be worth to fix. 🤔
Here is a video.
Yeah I've noticed this too @eugenekasimov. I think it's ok as is. It's the same behaviour we've had for the locale selectors in the footer. 🤷 |
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 Ludo!
Not a blocker
- I did find the selectors pretty wide but I guess this is due to the longest option in the list. The only thing I noticed is that the focus is shorter compared to the component's width (Visual reference)
PR Summary:
Adding the locale selector for the no-js behaviour in the header
Why are these changes introduced?
Fixes #2262
What approach did you take?
In the original PR, the no script behaviour was there at first so I re added it with some extra styling tweaks.
Other considerations
Visual impact on existing themes
Visual impact only on the no JS environment which is a very niche use case.
Testing steps/scenarios
Inspect
, click on the settings wheel in the inspect tool, scroll all the way to the bottom and find theDebugger > Disable JavaScript
Demo links
Checklist