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 6 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:
const reduceMotion = 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`
| duration: duration.short3, | ||
| easing: Easing.bezier(...easing.standard), | ||
| useNativeDriver: true, | ||
| }).start(); | ||
| }, [sortDirection, spinAnim]); | ||
| }, [sortDirection, spinAnim, duration, easing]); |
There was a problem hiding this comment.
- duration: duration.short3,
+ duration: duration.short3 * scale,The latest commit restored * scale in Banner, but the sort-arrow rotation still runs at full duration when PaperProvider has zeroed theme.animation.scale for reduce-motion. Not a regression — it was a hardcoded 150 before — but you're already editing this exact Animated.timing call, and leaving the two halves of one PR on different reduce-motion stories invites the next reader to pick the wrong one. Needs const { scale } = theme.animation; and the extra dep, same as Banner.
|
@MikitasK let me know when this is ready for another review! |
JKobrynski
left a comment
There was a problem hiding this comment.
LGTM! @satya164 @artus9033 all yours
|
@MikitasK we've got conflicts |
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