feat: add accessibility support to loading state components#388
feat: add accessibility support to loading state components#388adrienzheng-cb wants to merge 3 commits intomasterfrom
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
a0c602c to
f7567c4
Compare
f7567c4 to
5add6fd
Compare
5add6fd to
d02f2f9
Compare
d02f2f9 to
2ac7f52
Compare
| addPreviousPercent(progress); | ||
| const previousPercent = getPreviousPercent() ?? 0; | ||
|
|
||
| const animatedProgress = useRef(new Animated.Value(previousPercent)); |
There was a problem hiding this comment.
The entire usePreviousValue logic is redundant. We just need to initialize the animated progress value based on disableAnimateOnMount and that is it.
We also use the same simple logic in ProgressCircle component. Not sure why ProgrssBar was over-complicated
7b33f2b to
04a46ed
Compare
| percentage, | ||
| disableRandomRectWidth, | ||
| rectWidthVariant, | ||
| accessibilityLabel = 'Loading', |
There was a problem hiding this comment.
We might want to test this part out with @samcartersmith since a customer might have several dozen fallbacks on the page at a time.
There was a problem hiding this comment.
@hcopp That's why we went with this label vs an aria-live monstrous amount of noise. I think if there is a page with 20 fallbacks that are all persistent long enough for a user to arrow through them it will give a better understanding of the entire page is struggling to load.
349d63c to
907f793
Compare
Accessibility improvements: - Add role="status", aria-live="polite", and accessibilityLabel to Fallback, Spinner, and LottieStatusAnimation on web and mobile - LottieStatusAnimation auto-generates labels based on status - Update Fallback docs with accessibility guidance and grouping examples Bug fixes: - Fix ProgressBar callback re-animation issue by storing callbacks in refs instead of including them in useEffect dependencies Test improvements: - Add shape, width variant, and accessibility tests for Fallback - Add size variant and accessibility tests for Spinner - Add cardSuccess and status transition tests for LottieStatusAnimation Refactoring: - Move useFallbackShape hook to @coinbase/cds-common for cross-platform use - Use shapeBorderRadius tokens in RemoteImage/RemoteImageGroup - deprecated all exports from LottieStatusAnimationProps file in commons - renamed LottieStatusAnimationType to LottieStatus - removed redudant usePreviousValue logic from ProgressBar
907f793 to
ce7e005
Compare
6ac7a4d to
7c001bb
Compare
7c001bb to
40d9548
Compare
| ::: | ||
| ### Accessibility | ||
|
|
||
| Fallback includes a visually hidden "Loading" label by default, which screen readers will announce. You can customize this with the `accessibilityLabel` prop. Wrap Fallback in a live region container to announce loading state changes. |
There was a problem hiding this comment.
I'd suggest to avoid mentioning the default label. We want to encourage customers to pass in their own a11y labels for i18n support. Your example below helps to encourage this. The default label is just a fallback (pun not intended) in case a label isn't passed in. It's better than nothing but not the ideal label.
| Fallback includes a visually hidden "Loading" label by default, which screen readers will announce. You can customize this with the `accessibilityLabel` prop. Wrap Fallback in a live region container to announce loading state changes. | |
| Fallback has an `accessibilityLabel` prop to describe the loading state for assistive technologies. Wrap Fallback in a live region container to announce loading state changes. |
| } | ||
| ``` | ||
|
|
||
| For multiple Fallbacks, add `aria-hidden` to all of them and provide your own visually hidden label: |
There was a problem hiding this comment.
Small nit, can we rephrase this to inform customers about the prop but not require its usage? Using aria-hidden is helpful in some cases but in others may not be ideal.
| For multiple Fallbacks, add `aria-hidden` to all of them and provide your own visually hidden label: | |
| `aria-hidden` can be passed into a Fallback. This can be useful if you render multiple Fallbacks in an area and would prefer to have a parent container have the aria-label needed for the accessibility API. Whether it's a parent container or each Fallback, there needs to be an element with an `aria-label` indicating a loading state. |
Accessibility improvements:
Fallback, Spinner, and LottieStatusAnimation on web and mobile
Test improvements:
Refactoring:
What changed? Why?
Root cause (required for bugfixes)
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false