-
Notifications
You must be signed in to change notification settings - Fork 61
feat: add accessibility support to loading state components #388
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
base: master
Are you sure you want to change the base?
Conversation
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 - Add LottieAccessibilityProps types to animation types Also includes auto-generated icon updates (autoCar, birthcertificate, webhooks) and pencil icon active/inactive swap fix. Co-authored-by: Cursor <[email protected]>
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
| }; | ||
|
|
||
| export type LottieBaseProps<T extends LottieSource = LottieSource> = Omit< | ||
| BoxBaseProps, |
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 optional new props are added, non-breaking
| const animatedProgress = useRef(new Animated.Value(previousPercent)); | ||
|
|
||
| const [innerWidth, setInnerWidth] = useState<number>(-1); | ||
| const [trackWidth, setTrackWidth] = useState<number>(-1); |
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.
changed to more accurate variable name
| const onAnimationEndRef = useRef(onAnimationEnd); | ||
| const onAnimationStartRef = useRef(onAnimationStart); | ||
| onAnimationEndRef.current = onAnimationEnd; | ||
| onAnimationStartRef.current = onAnimationStart; |
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.
ensure the ref is up-to-date whenever component rerenders. we should switch to useEffectCallback after react 19
| testID, | ||
| accessibilityLabel, | ||
| ...otherProps | ||
| }: LottieStatusAnimationProps & { accessibilityLabel?: string }) => { |
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 wonder if we missed bringing this to web and mobile during v8 migration.
You think we would be better off here bringing this into mobile and web? Also we could do
import { LottieStatusAnimationProps as CommonLottieStatusAnimationProps, SharedAccessibilityProps } from ...
type LottieStatusAnimationProps = CommonLottieStatusAnimationProps & {
/**
* Accessibility label shown along with the current state
* @default 'Loading' when loading, ... (or whatever you want to say)
*/
accessibilityLabel?: SharedAccessibilityProps['accessibilityLabel'];
}
This way people would know to override for i18n as well.
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 thought v8 was trying to get away from defining common props in common package
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.
Yep that's why I'm surprised we missed this in v8.
| /** When true, indicates that the view is an accessibility element */ | ||
| accessible?: boolean; | ||
| /** A string value that labels an interactive element for VoiceOver */ | ||
| accessibilityLabel?: string; |
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.
Nit: could we pull from SharedAccessibilityProps or from BoxProps?
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 attempted to do that, but it does not include role or aria-live prop :(.
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.
Could we do LottieBaseProps<...> = Pick<SharedAccessibilityProps, 'accessibilityLabel> & ...?
I'm not seeing role or aria-live in this spot, I know I saw it elsewhere
| import type { Polymorphic } from '../core/polymorphism'; | ||
|
|
||
| import { Box, type BoxBaseProps } from './Box'; | ||
|
|
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.
updated to use the same useFallbackShape hook and type defined in common
| /** | ||
| * A boolean flag indicating whether or not the animation should start automatically when | ||
| * mounted. This only affects the imperative API. | ||
| * @default false |
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.
nit: could we remove these falsy defaults
|
|
||
| Fallback.displayName = 'Fallback'; | ||
|
|
||
| export { useFallbackShape, type UseFallbackShapeOptions }; |
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.
re-export them for backward-compatibility
| hexagon: css` | ||
| border-radius: 0; | ||
| border-radius: ${shapeBorderRadius.hexagon}px; | ||
| `, |
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.
remove hardcoded borderRadius value
| const onAnimationEndRef = useRef(onAnimationEnd); | ||
| const onAnimationStartRef = useRef(onAnimationStart); | ||
| onAnimationEndRef.current = onAnimationEnd; | ||
| onAnimationStartRef.current = onAnimationStart; |
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 exactly was happening in your tests here?
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.
Nice
| role?: AriaRole; | ||
| /** ARIA live region setting for dynamic content announcements */ | ||
| 'aria-live'?: AriaAttributes['aria-live']; | ||
| }; |
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.
Do we want to include this as a separate type? Is this being used elsewhere or only in this file?
Accessibility improvements:
Bug fixes:
Test improvements:
Refactoring:
Also includes auto-generated icon updates (autoCar, birthcertificate, webhooks) and pencil icon active/inactive swap fix.
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