-
Notifications
You must be signed in to change notification settings - Fork 382
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
#10158 painless accessibility improvements #10159
base: master
Are you sure you want to change the base?
#10158 painless accessibility improvements #10159
Conversation
On Behalf of DB Systel
Too much Zoom does seem to break the layout, but handing that responsibility to the end user is more accessible than disabling it. Map zoom is unaffected, and since using browser zoom enlarges map controls, users who tried zooming into the map using browser zoom should be able to notice the actual controls and recover from their mistake. On Behalf of DB Systel
- Convert heading that, semantically, should not be a heading, to a div - correctly assign label to scalebox - add alt tag to attribution logo On Behalf of DB Systel
@fkellner thank you so much for your contribution. We will review as soon as possible. |
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.
@fkellner thanks for the contribution, most of the changes are good, there is just to review how we set the lang in the Localized component
@@ -37,6 +37,7 @@ class Localized extends React.Component { | |||
if (typeof children === 'function') { | |||
children = children(); | |||
} | |||
document.documentElement.setAttribute("lang", this.props.locale); |
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.
It would be better to move this in componentDidMount
and componentDidUpdate
and remove it from the render
method, it could be something like:
componentDidMount() {
this.updateDocumentLangAttribute();
}
componentDidUpdate(prevProps) {
if (this.props.locale !== prevProps.locale) {
this.updateDocumentLangAttribute();
}
}
updateDocumentLangAttribute() {
if (document?.documentElement) {
document.documentElement.setAttribute("lang", this.props.locale);
}
}
Description
Tiny changes that improve accessibility. Should hopefully not take longer than a minute to review.
What kind of change does this PR introduce?
Issue
What is the current behavior?
#10158
Bad Lighthouse Accessibility Score
What is the new behavior?
Better Lighthouse Accessibility Score through:
Breaking change
Does this PR introduce a breaking change?
Other useful information
Too much Zoom does seem to break the layout, but handing that responsibility to the end user is more accessible for people who need larger controls than
disabling it. Map zoom is unaffected, and since using browser zoom enlarges map controls, users who tried zooming
into the map using browser zoom should be able to notice the actual controls and recover from their mistake.