feat(menu): modernize Menu to Material Design 3 specs - #5037
Conversation
Menu.Item used fixed 48dp height with two-line supporting text and a content minWidth that collapsed trailingSupportingText to zero width. Use minHeight + padding for supporting rows, intrinsic row sizing for trailing slots, and showcase the vibrant color scheme in the example.
MikitasK
left a comment
MikitasK
left a comment
There was a problem hiding this comment.
great work on this modernization 👏
just a few suggestions to consider before merging the PR:
| const { titleColor, iconColor, contentOpacity } = getMenuItemColor({ | ||
| const layout = useMenuItemLayout(); | ||
|
|
||
| const colorScheme = colorSchemeProp ?? layout?.colorScheme ?? 'standard'; |
There was a problem hiding this comment.
what about using MenuRootContext as a fallback here?
I suppose, items rendered through wrapper / Fragment won't receive MenuItemLayoutContext & will fall back to standard scheme even inside vibrant menu
| const colorScheme = colorSchemeProp ?? layout?.colorScheme ?? 'standard'; | |
| const root = useMenuRoot(); | |
| const colorScheme = colorSchemeProp ?? layout?.colorScheme ?? root?.colorScheme ?? 'standard'; |
| > | ||
| {title} | ||
| </Text> | ||
| {supportingText ? ( |
There was a problem hiding this comment.
could use explicit presence check for supportingText & trailingSupportingText?
this way values like supportingText={0} will be rendered
instead, we can define such variable:
const hasSupportingText = supportingText != null && typeof supportingText !== 'boolean';
and then use it here
| {supportingText ? ( | |
| {hasSupportingText ? ( |
| </Text> | ||
| ) : null} | ||
| </View> | ||
| {trailingSupportingText ? ( |
| ...(supportingText | ||
| ? { paddingVertical: 8 } | ||
| : { height: dense ? denseItemHeight : itemHeight }), |
There was a problem hiding this comment.
could we allow the item to grow when trailing supporting text is present?
it currently keeps fixed 48/32dp height & might be clipped with large accessibility font sizes
we can define the following vars first:
const hasSupportingText = supportingText != null && typeof supportingText !== 'boolean';
const hasTrailingSupportingText = trailingSupportingText != null && typeof trailingSupportingText !== 'boolean';
and then use it like that:
| ...(supportingText | |
| ? { paddingVertical: 8 } | |
| : { height: dense ? denseItemHeight : itemHeight }), | |
| ...(hasSupportingText || hasTrailingSupportingText | |
| ? { paddingVertical: 8 } | |
| : { height: dense ? denseItemHeight : itemHeight }), |
| const contentMaxWidth = getContentMaxWidth({ | ||
| iconWidth: iconSize, | ||
| leadingIcon, | ||
| trailingIcon, | ||
| hasTrailingSupportingText: Boolean(trailingSupportingText), | ||
| }); |
There was a problem hiding this comment.
could we use explicit presence flags for these props?
this approach should keep numeric values (such as 0), but ignore boolean placeholders
| const contentMaxWidth = getContentMaxWidth({ | |
| iconWidth: iconSize, | |
| leadingIcon, | |
| trailingIcon, | |
| hasTrailingSupportingText: Boolean(trailingSupportingText), | |
| }); | |
| const hasTrailingSupportingText = trailingSupportingText != null && typeof trailingSupportingText !== 'boolean'; | |
| const contentMaxWidth = getContentMaxWidth({ | |
| iconWidth: iconSize, | |
| leadingIcon, | |
| trailingIcon, | |
| hasTrailingSupportingText, | |
| }); |
2 participants
Motivation
Modernize
Menu/Menu.Itemto the latest Material Design 3 specs. Reuse theme tokens (shape, typography, motion, color roles) and extract component tokens in the same pattern as Checkbox / FAB / ConnectedButtonGroup.Spec re-check (M3 menus)
Re-checked M3 menus specs and overview:
surfaceContainerLow. Theelevationprop controls shadow only (Paper’selevation.level2maps to different surfaceContainer tones, so it is not used as the menu fill).colorScheme="vibrant"; spring-animated item corner morph as focus/selection moves; optional badge and lightweight submenu portal surface; Menu.Section group gaps (8dp) in addition to Divider.Changes
Tokens / Menu / Menu.Item
surfaceContainerLow; surfacecorner.large; first/last/selected itemcorner.mediumvia layout context (nocloneElement/ nodisplayNamefiltering)labelLarge, supporting + trailing supporting text, selected colors (disabled wins with content opacity)toRawSpring+ reduce-motion snapcolorSchemestandard | vibrantMenu.Sectiongroup gaps; badge; lightweight submenu portal;Animated.springper-corner radius morph on focus/selectionExample / docs / tests
Related issue
Fixes #4977
Test plan
yarn typecheck/yarn lint/ Menu unit testsVisual verification
Docs screenshots:
docs/public/screenshots/menu-*.pngandmenu-ios-*.png.