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 |
| const { backgroundColor, ...restStyle } = StyleSheet.flatten(style) || {}; | ||
| const { background, textColor } = resolveAvatarColors({ | ||
| theme, | ||
| backgroundColor, | ||
| color: rest.color, | ||
| }); |
There was a problem hiding this comment.
Avoid reading StyleSheet.flatten. If user provides a custom background color, they should also provide a custom text color.
Color parsing can't handle PlatformColor, CSS custom properties etc. Remove any extra logic like this.
Also update migration guide once it's changed
| const theme = useInternalTheme(themeOverrides); | ||
| const { backgroundColor } = StyleSheet.flatten(style) || {}; | ||
| const { background } = resolveAvatarColors({ theme, backgroundColor }); |
| const { backgroundColor, ...restStyle } = StyleSheet.flatten(style) || {}; | ||
| const { background, textColor } = resolveAvatarColors({ | ||
| theme, | ||
| backgroundColor, | ||
| color: customColor, | ||
| }); |
| {icon} | ||
|
|
||
| <Text | ||
| variant="labelMedium" |
There was a problem hiding this comment.
This won't use the passed theme overrides to DataTableTitle. Pass the overrides to Text as well.
| </View> | ||
| )} | ||
| <Text | ||
| variant="bodySmall" |
There was a problem hiding this comment.
Same comment as DataTableTitle about theme overrides
| style={styles.optionsContainer} | ||
| > | ||
| <Text | ||
| variant="bodySmall" |
There was a problem hiding this comment.
Same comment as DataTableTitle about theme overrides
|
|
||
| return ( | ||
| <Text | ||
| variant="bodyMedium" |
There was a problem hiding this comment.
Same comment as DataTableTitle about theme overrides
| // A disabled animation scale settles within a frame instead of taking the | ||
| // full `motion.duration.medium1`. |
There was a problem hiding this comment.
Remove the AI generated comments. Whether it takes motion.duration.medium1 or something else with animation is irrelevant to this test.
| const sortIcon = () => { | ||
| const node = screen.getByText('arrow-up', { | ||
| includeHiddenElements: true, | ||
| }).parent; | ||
|
|
||
| if (!node) { | ||
| throw new Error('Sort icon not found'); | ||
| } | ||
|
|
||
| return node; | ||
| }; | ||
|
|
||
| it('uses zero-duration sort animation when animation scale is disabled', async () => { | ||
| const view = await render( | ||
| <DataTable.Title | ||
| sortDirection="ascending" | ||
| theme={{ animation: { scale: 0 } }} | ||
| > | ||
| Dessert | ||
| </DataTable.Title> | ||
| ); | ||
|
|
||
| await view.rerender( | ||
| <DataTable.Title | ||
| sortDirection="descending" | ||
| theme={{ animation: { scale: 0 } }} | ||
| > | ||
| Dessert | ||
| </DataTable.Title> | ||
| ); | ||
| await act(() => { | ||
| jest.advanceTimersByTime(16); | ||
| }); | ||
|
|
||
| // A disabled animation scale lands on the final rotation within a frame. | ||
| expect(getAnimatedStyle(sortIcon())).toMatchObject({ | ||
| transform: [{ rotate: '180deg' }], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
remove this test. don't inspect internal elements.
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