-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix(ADA-1751): [TR] ADA Loading play not announced #968
base: master
Are you sure you want to change the base?
Conversation
src/components/spinner/spinner.tsx
Outdated
loadingSpan: RefObject<HTMLElement> = createRef<HTMLElement>(); | ||
|
||
componentDidMount() { | ||
setTimeout(() => { |
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.
what does the setTimeout do ?
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.
only if the loading appears more than one second screen reader will announce: "loading" (asked by product)
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.
two things
- you need to start the timer from the moment the loading is VISIBLE, not mounted
- i didn't understand the use of ref here. is using a boolean not good enough ?
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 checked it and when the setTimeout is started, loading is already visible.
- need to update the span with the "loading" message if we want the screen reader to announce it immediately. in order to do it need to save the span in ref and update it.
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 would still add a visibility check. i don't know if you can count on it.
- what happens if you don't use a ref ?
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.
visibility check was added.
src/components/spinner/spinner.tsx
Outdated
loadingSpan: RefObject<HTMLElement> = createRef<HTMLElement>(); | ||
|
||
componentDidMount() { | ||
setTimeout(() => { |
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.
two things
- you need to start the timer from the moment the loading is VISIBLE, not mounted
- i didn't understand the use of ref here. is using a boolean not good enough ?
src/components/spinner/spinner.tsx
Outdated
loadingSpan: RefObject<HTMLElement> = createRef<HTMLElement>(); | ||
|
||
componentDidMount() { | ||
setTimeout(() => { |
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 would still add a visibility check. i don't know if you can count on it.
- what happens if you don't use a ref ?
Description of the Changes
Please add a detailed description of the change, whether it's an enhancement or a bugfix.
If the PR is related to an open issue please link to it.
Issue:
Screen reader user doesn't have indication on loading state.
Fix:
Adding hidden element that visible only to screen reader. in case loading take more than second, "loading" message will be announced.
Resolves ADA-1751