refactor: searchbar component - #5020
Conversation
MikitasK
left a comment
There was a problem hiding this comment.
great job 👍
just a few things comments from my side:
|
|
||
| return ( | ||
| <Reanimated.View | ||
| style={applyFocusMargin ? containedMarginStyle : null} | ||
| testID={`${testID}-wrapper`} | ||
| > | ||
| <Surface | ||
| style={[ | ||
| styles.container, | ||
| { backgroundColor: containerColor, borderRadius }, | ||
| style, | ||
| ]} | ||
| testID={`${testID}-container`} | ||
| elevation={elevation} | ||
| container | ||
| theme={theme} | ||
| > |
There was a problem hiding this comment.
could we apply parent-facing layout styles to this wrapper?
currently flex, position, alignSelf & similar props remain on nested Surface, so they no longer affect Searchbar relative to its parent
we could split layout & visual styles using existing splitStyles helper
| return ( | |
| <Reanimated.View | |
| style={applyFocusMargin ? containedMarginStyle : null} | |
| testID={`${testID}-wrapper`} | |
| > | |
| <Surface | |
| style={[ | |
| styles.container, | |
| { backgroundColor: containerColor, borderRadius }, | |
| style, | |
| ]} | |
| testID={`${testID}-container`} | |
| elevation={elevation} | |
| container | |
| theme={theme} | |
| > | |
| const [surfaceStyle, wrapperStyle] = splitStyles(flatStyle || {}, (key) => key.startsWith('margin') || ['position', 'alignSelf', 'top', 'right' ...].includes(key) | |
| ); | |
| return ( | |
| <Reanimated.View | |
| style={[wrapperStyle, applyFocusMargin ? containedMarginStyle : null]} | |
| testID={`${testID}-wrapper`} | |
| > | |
| <Surface | |
| style={[ | |
| styles.container, | |
| { backgroundColor: containerColor, borderRadius }, | |
| surfaceStyle, | |
| ]} | |
| testID={`${testID}-container`} | |
| elevation={elevation} | |
| container | |
| theme={theme} | |
| > |
| const HORIZONTAL_MARGIN_KEYS = [ | ||
| 'margin', | ||
| 'marginHorizontal', | ||
| 'marginLeft', | ||
| 'marginRight', | ||
| 'marginStart', | ||
| 'marginEnd', | ||
| ] as const; |
There was a problem hiding this comment.
what about including logical horizontal margin props here?
marginInline, marginInlineStart, marginInlineEnd are supported by RN, but currently they don’t disable built-in focus margin (RN docs)
| const HORIZONTAL_MARGIN_KEYS = [ | |
| 'margin', | |
| 'marginHorizontal', | |
| 'marginLeft', | |
| 'marginRight', | |
| 'marginStart', | |
| 'marginEnd', | |
| ] as const; | |
| const HORIZONTAL_MARGIN_KEYS = [ | |
| 'margin', | |
| 'marginHorizontal', | |
| 'marginLeft', | |
| 'marginRight', | |
| 'marginStart', | |
| 'marginEnd', | |
| 'marginInline', | |
| 'marginInlineStart', | |
| 'marginInlineEnd', | |
| ] as const; |
MikitasK
left a comment
There was a problem hiding this comment.
- updated implementation to preserve consumer layout styles on outer wrapper while keeping focus-margin animation on a separate Reanimated view
- expanded
HORIZONTAL_MARGIN_KEYSto includemarginInline,marginInlineStart,marginInlineEnd - added regression tests & updated snapshots
| // component (input was onSurfaceVariant, placeholder was onSurface). | ||
| input: 'onSurface', | ||
| placeholder: 'onSurfaceVariant', | ||
| leadingIcon: 'onSurfaceVariant', |
There was a problem hiding this comment.
md.comp.search-bar.leading-icon.color resolves to md.sys.color.on-surface, not on-surface-variant, and the view namespace agrees (md.comp.search-view.header.leading-icon.color → on-surface). The PR fixes two of the three roles; this is the third.
Watch the knock-on. Searchbar.tsx:284 derives iconColor from leadingIconColor, and :450 reuses it for the clear button, which per md.comp.search-bar.trailing-icon.color should stay on-surface-variant. It's invisible today only because both roles resolve alike.
- iconColor={value ? iconColor : 'rgba(255, 255, 255, 0)'}
+ iconColor={value ? trailingIconColor : 'rgba(255, 255, 255, 0)'}| const styles = StyleSheet.create({ | ||
| container: { | ||
| width: '100%', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The new results surface ships square corners. md.comp.search-view.contained.docked.results.shape → md.sys.shape.corner.medium = 12dp. (The 28dp corner.extra-large belongs to md.comp.search-view.docked.container.shape, which is the whole docked view, not the results block.)
Everything else in the component routes shape through SearchbarTokens + resolveCornerRadius, so this wants a results: 'medium' entry in tokens.ts rather than a bare style.
| * Search layout mode, the default value is "contained". | ||
| * - `contained` - the recommended M3 Expressive style: a rounded, elevated | ||
| * bar whose horizontal margins animate from 24dp down to 12dp on focus | ||
| * (grow-wider effect). Providing any horizontal margin via `style` | ||
| * replaces the built-in margin and disables the focus transition. | ||
| * - `divided` - a full-bleed search view with square corners and a bottom | ||
| * `Divider`. Deprecated in M3 Expressive in favor of `contained`. | ||
| */ | ||
| mode?: 'contained' | 'divided'; |
There was a problem hiding this comment.
docs/6.x/docs/guides/migration.md has a ## Components section (still only TextInput) and this PR adds nothing to it. The before/after snippet already in the PR body needs to live in the guide.
Please cover both breaks. The behavioural one is the bigger surprise: bar was full-bleed, contained inserts 24dp horizontal margins, so every existing <Searchbar /> moves. And the opt-out is undiscoverable - HORIZONTAL_MARGIN_KEYS (:74-84) includes margin, so style={{ margin: 4 }} silently disables the focus animation. Your own example had to switch to marginVertical: 4 (SearchbarExample.tsx:240) for exactly that reason.
| const hasWrapperStyle = Object.keys(wrapperStyle).length > 0; | ||
|
|
||
| return ( | ||
| <Animated.View |
There was a problem hiding this comment.
This is React Native's Animated, now rendered at runtime - on main the import was used only in type positions. The "preserve Searchbar layout during focus animation" commit moved transform onto the outer wrapper, and the test at Searchbar.test.tsx:75-100 drives it with an Animated.Value, so the node has to be an RN Animated.View.
House rule is Reanimated only, and #5062's own note about animated elevation staying broken on web until the move to Reanimated says which way this is meant to travel.
|
@MikitasK can you resolve the comments that were addressed? |
| * Set style of the TextInput component inside the searchbar | ||
| */ | ||
| inputStyle?: StyleProp<TextStyle>; | ||
| style?: StyleProp<ViewStyle>; |
There was a problem hiding this comment.
Shouldn't that be StyleProp<AnimatedStyle<ViewStyle>> just like in #5078?
|
@MikitasK just one comment this time ☝️ |
Motivation
Modernizes the Searchbar to the latest MD3 (Expressive) search spec by adopting the shared theme tokens (shape, motion, colors), extracting component-specific tokens, and adding the missing results container - following the token/structure conventions of the already-modernized TextInput/Switch/Checkbox/FAB. Also fixes the long-standing swapped input/placeholder colors and adds the missing focus transition.
Changes
Colors (tokens.ts / utils.ts)
Shape
Motion - focus transition (new)
barmode was full-width. Set a horizontal margin instyleto override it — that also turns the focus animation off.Structure
index.tsx (attaches Results as a static sub-component) - mirroring Switch/Checkbox/FAB.
New sub-component — Searchbar.Results (SearchbarResults.tsx)
Breaking change
Mode values are renamed to MD3 terminology
Related issue
#4978
Test plan
searchbar.mp4