Conversation
MikitasK
left a comment
There was a problem hiding this comment.
nice work! 👍 overall, this looks like a solid foundation for the new Toolbar components
just a few points to address before merge:
JKobrynski
left a comment
There was a problem hiding this comment.
I think it's the last one, and it's minor
|
LGTM! @satya164 do you want to take a look? |
87be2b8 to
d4845c7
Compare
| @@ -0,0 +1,3388 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
There was a problem hiding this comment.
I think you need to rerun these with -u after rebasing, because default testIDs were removed
There was a problem hiding this comment.
You were right, the PR was still behind main branch. I rebased it and now it is up to date.
| * Style for positioning `floating`'s pill, or overriding `docked`'s | ||
| * default anchoring. | ||
| */ | ||
| style?: StyleProp<ViewStyle>; |
There was a problem hiding this comment.
What happens when you pass backgroundColor|borderRadius through this prop? I'm afraid they'd silently do nothing, other components type style as styleProp<SurfaceStyle>
There was a problem hiding this comment.
You are correct, fixed it by changing style type to StyleProp<SurfaceStyle>
| import { resolveCornerRadius } from '../../theme/utils/shape'; | ||
| import Surface from '../Surface'; | ||
|
|
||
| export type Props = { |
There was a problem hiding this comment.
Other components use Omit<ViewProps, 'style'> to allow props like onLayout`, does that work here?
There was a problem hiding this comment.
Fixed it, changed Props to Omit<ViewProps, 'style'>
| ? [styles.verticalAnchor, { right: insets.right + 16 }] | ||
| : [styles.horizontalAnchor, { bottom: insets.bottom + 24 }] |
There was a problem hiding this comment.
Have you checked the edge offsets against the toolbar specs? I'm afraid the two are swapped
There was a problem hiding this comment.
The horizontal edge offset is incorrect, it should be 16. As for the vertical one the only spec in MD3 is that the minimum offset from screen edge should be equal to 16 so I left it as it is.
d4845c7 to
d24d47c
Compare
| ref={ref} | ||
| pointerEvents="box-none" | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| style={[styles.dockedContainer, style as StyleProp<ViewStyle>]} |
There was a problem hiding this comment.
Do we really need to use as here?
There was a problem hiding this comment.
Yes, it's needed. style is typed as StyleProp<SurfaceStyle>, and SurfaceStyle wraps ViewStyle in Reanimated's AnimatedStyle (so it typechecks when forwarded to Surface, which is an Animated.View). Here it's applied to a plain View instead, which only accepts plain ViewStyle - hence the mismatch.
The cast is safe in practice: this wrapper isn't animated, so no shared-value-typed style ever reaches it. I also checked whether swapping this to Reanimated's Animated.View would avoid the cast - it does typecheck, but it always renders with collapsable={false}, which is a permanent Android view-flattening cost for every Toolbar (docked or not) just to accommodate a case that doesn't apply here.
Motivation
Introduces
Toolbar, implementing the Material Design 3 toolbars spec. It reuses existing theme tokens (shape, color roles, elevation) and follows the same per-component-tokens pattern asFAB/Checkbox.Spec re-check (M3 toolbars)
Re-checked the M3 toolbars spec:
variants:floating(self-positioned pill,corner.full, elevation level 3) anddocked(full-width bar pinned to the bottom edge,corner.none, no elevation, extends into safe-area insets).floatingsupportshorizontal/verticalorientation;dockedis always horizontal per spec.standard(default) colors: container/unselected-buttonsurfaceContainer, icon/labelonSurfaceVariant, selected buttonsecondaryContainerwithonSecondaryContainericon.vibrantcolorScheme: container/unselected-buttonprimaryContainer, icon/labelonPrimaryContainer, selected button falls back tosurfaceContainerwithonSurfaceicon.IconButton/Buttondescendants pick up the toolbar'scolorSchemeautomatically via context - not just direct children - unless they already set their ownmodeor an explicit color prop.Changes
Toolbar/ tokens / utilssrc/components/Toolbar/{Toolbar.tsx,tokens.ts,utils.ts,ToolbarColorContext.ts}:variant(floating/docked),orientation(horizontal/vertical, floating-only),colorScheme(standard/vibrant),containerColoroverride,style/contentContainerStyle,testID,aria-label,theme,refToolbarTokens.docked's outer wrapper has no inset margin, so its background naturally spans edge-to-edge into the safe area, while the content row getsinsets-based margin to stay clear of it - the 64dp icon band itself stays untouchedToolbarColorContextcarriestheme/colorSchemedown to descendants;IconButton/Buttoneach read it directly and resolve their own color, backing off if they already have amodeor explicit color propsrc/index.tsxExample / docs / tests
ToolbarExample.tsx) covering both variants, both orientations, both color schemes, over a scrollable listToolbar.test.tsx) covering shape/elevation per variant, color resolution across light/dark themes, and child auto-recoloring (including nested/non-direct descendants, opt-out viamodeor explicit color)Scope note
Per Satyajit Sahoo's recommendation, this lands as a series of PRs instead of one large change. This is PR 1 of the series:
colorSchemestandard/vibrant. Component, tokens, tests, docs, example.Related issue
Related to #4988
Test plan
yarn typecheck/yarn lint/ Toolbar unit testsVisual verification