Uh oh!
There was an error while loading. Please reload this page.
refactor: modernize non standard components to the latest MD3 spec - #5016
refactor: modernize non standard components to the latest MD3 spec#5016MrMuzyk wants to merge 4 commits into
Conversation
MikitasK
left a comment
There was a problem hiding this comment.
nicely done 👏
just one comment to consider before merge:
| // show | ||
| Animated.timing(position, { | ||
| duration: 250 * scale, | ||
| duration: duration.medium1, |
There was a problem hiding this comment.
could we keep using theme.animation.scale here? PaperProvider sets it to 0 here for reduced motion
| duration: duration.medium1, | |
| duration: duration.medium1*scale, |
| // hide | ||
| Animated.timing(position, { | ||
| duration: 200 * scale, | ||
| duration: duration.short4, |
There was a problem hiding this comment.
same here
| duration: duration.short4, | |
| duration: duration.short4*scale, |
| @@ -159,20 +161,22 @@ const Banner = ({ | |||
| if (visible) { | |||
| // show | |||
| Animated.timing(position, { | |||
| duration: 250 * scale, | |||
| duration: duration.medium1 * scale, | |||
| toValue: 1, | |||
| useNativeDriver: false, | |||
| easing: Easing.bezier(...easing.standard), | |||
| }).start(showCallback); | |||
| } else { | |||
| // hide | |||
| Animated.timing(position, { | |||
| duration: 200 * scale, | |||
| duration: duration.short4 * scale, | |||
| toValue: 0, | |||
| useNativeDriver: false, | |||
| easing: Easing.bezier(...easing.standard), | |||
| }).start(hideCallback); | |||
| } | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| }, [visible, position, scale]); | |||
| }, [visible, position, duration, easing, scale]); | |||
There was a problem hiding this comment.
- const { scale } = theme.animation;+ const { duration, easing } = theme.motion; ... - duration: 250 * scale,+ duration: duration.medium1,
Dropping the * scale multiplier turns off reduce-motion for Banner entirely. scale isn't a legacy leftover — PaperProvider.tsx:35-37 sets it to 0 whenever useResolvedReduceMotion() resolves true, either from AccessibilityInfo.isReduceMotionEnabled or from the explicit reduceMotion prop. 250 * scale was therefore 0 for those users and the banner snapped in and out; after this change it always runs the full 250ms/200ms transition. The token adoption
itself is fine (medium1 = 250, short4 = 200, identical to the old numbers), it's only the multiplier that carries the a11y behaviour.
The PR description says this matches "how Switch/Checkbox adopted motion tokens", but those two didn't drop reduce-motion — they replaced the mechanism. Switch.tsx:169-171 and Checkbox.tsx both call useReduceMotion() from src/theme/accessibility/ReduceMotionContext and feed ReduceMotion.Always/Never into their Reanimated configs. Banner ends up honouring neither path, and it's now the only animated component in src/ that reads neither theme.animation.scale (still live in ProgressBar, Modal, Badge, Snackbar, ActivityIndicator, Chip, Card, CrossFadeIcon, DrawerCollapsedItem, RadioButtonAndroid) nor useReduceMotion().
RN's Animated.timing has no reduceMotion option, so either keep scale, or take the Switch/Checkbox route:
constreduceMotion=useReduceMotion();// ...duration: reduceMotion ? 0 : duration.medium1,There was a problem hiding this comment.
thanks for raising this up @JKobrynski 🙏
basically, this issue was addressed in recent ae2ecfc commit. both Banner durations now use theme.animation.scale & regression test covers scale: 0
as for PR description, it's stale & I can't update since I'm not the owner of this PR
but here's what it should say instead:
### Banner — Motion
- Durations `250 * scale` / `200 * scale` -> `theme.motion.duration.medium1 * scale` / `theme.motion.duration.short4 * scale` (values unchanged at the default scale) + `Easing.bezier(...theme.motion.easing.standard)`
- Banner continues to respect `theme.animation.scale` including `scale: 0` reduced-motion behavior
### Visual / behavioral changes
- Banner & DataTable animations now use MD3 standard easing curve. Banner continues to honor `theme.animation.scale`
| @@ -159,20 +161,22 @@ const Banner = ({ | |||
| if (visible) { | |||
| // show | |||
| Animated.timing(position, { | |||
| duration: 250 * scale, | |||
| duration: duration.medium1 * scale, | |||
| toValue: 1, | |||
| useNativeDriver: false, | |||
| easing: Easing.bezier(...easing.standard), | |||
| }).start(showCallback); | |||
| } else { | |||
| // hide | |||
| Animated.timing(position, { | |||
| duration: 200 * scale, | |||
| duration: duration.short4 * scale, | |||
| toValue: 0, | |||
| useNativeDriver: false, | |||
| easing: Easing.bezier(...easing.standard), | |||
| }).start(hideCallback); | |||
| } | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| }, [visible, position, scale]); | |||
| }, [visible, position, duration, easing, scale]); | |||
There was a problem hiding this comment.
thanks for raising this up @JKobrynski 🙏
basically, this issue was addressed in recent ae2ecfc commit. both Banner durations now use theme.animation.scale & regression test covers scale: 0
as for PR description, it's stale & I can't update since I'm not the owner of this PR
but here's what it should say instead:
### Banner — Motion
- Durations `250 * scale` / `200 * scale` -> `theme.motion.duration.medium1 * scale` / `theme.motion.duration.short4 * scale` (values unchanged at the default scale) + `Easing.bezier(...theme.motion.easing.standard)`
- Banner continues to respect `theme.animation.scale` including `scale: 0` reduced-motion behavior
### Visual / behavioral changes
- Banner & DataTable animations now use MD3 standard easing curve. Banner continues to honor `theme.animation.scale`
Motivation
Modernizes the remaining non-standard components — Avatar, Banner, and DataTable — to the latest MD3 spec by adopting the recently added theme tokens (shape, motion, typography, colors).
Changes
Avatar (AvatarIcon, AvatarText, AvatarImage, new utils.ts)
Banner (Banner.tsx)
DataTable (DataTableTitle, DataTableCell, DataTablePagination)
Visual / behavioral changes (no public API changed)
Related issue
#4990
Test plan
yarn typescriptyarn lintyarn testbanner.mp4