-
Notifications
You must be signed in to change notification settings - Fork 56
fix: support disable animate on mount across progress bar components #284
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
Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
|
|
||
| <Text variant="label2">Disable animation on mount</Text> | ||
| <ProgressBarWithFixedLabels | ||
| disableAnimateOnMount |
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.
Adding this twice is a little annoying but it works this way for disable as well.
| /> | ||
| </HStack> | ||
| <HStack gap={2}> | ||
| <HStack gap={2} flexWrap="wrap"> |
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.
Was fixing some responsiveness issues on the docs page on my phone
| checksum: 10c0/d8cddf817d5bec8e7c2106affdbf1bfc3923463ca16697c992b2efeb043e6a5d9dcb70cda913bc6acf9112fd66f9e80279316c08e7800359116925066a63fdfa | ||
| version: 1.0.30001762 | ||
| resolution: "caniuse-lite@npm:1.0.30001762" | ||
| checksum: 10c0/93707eac5b0240af3f2ce6e2d7ab504a6fefcf9c2f9cd8fb9d488e496a333c61e557dab0472c1b00c17bc386a5dbb792aa4c778cda2d768e17f986617d7aec53 |
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.
Updating browserslist
| usePreviousValues<number>([disableAnimateOnMount ? progress : 0]); | ||
| const [size, onLayout] = useLayout(); | ||
| const containerWidth = size.width; | ||
| const [hasAnimationMounted, setHasAnimationMounted] = useState(!disableAnimateOnMount); |
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 wasn't able to get a clean animation on web and mobile that didn't have an initial mount state but we need to also get the container size first
maximo-macchi-cb
left a comment
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.
Great work! Just some small comments to address
| await waitFor(() => { | ||
| expect(bar).toHaveStyle({ | ||
| transform: 'translateX(-50%) translateZ(0)', | ||
| }); | ||
| }); |
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.
Is this assertion guaranteed to be checked right as the component is mounted? If you remove disableAnimateOnMount from the example <ProgressBar>, does the test fail?
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.
Good catch, I have adjusted the tests to check style right away and compare with disableAnimateOnMount prop.
| Apache-2.0 License | ||
|
|
||
| Copyright 2025 Coinbase | ||
| Copyright 2026 Coinbase |
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.
Happy new year! 🎉
What changed? Why?
This PR brings consistent support to disable animate on mount to progress bar and progress circle across web and mobile, after #280 shown that the prop existed but wasn't being used for progress circle.
Root cause (required for bugfixes)
While the prop existed in some spots, we weren't supporting it. See #280.
UI changes
Mobile
Web
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