Repository navigation
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 ☝️ |
|
|
||
| // A consumer-provided horizontal margin wins over the built-in one and | ||
| // disables the focus transition. | ||
| const flatStyle = StyleSheet.flatten<ViewStyle>(style); |
There was a problem hiding this comment.
Widening the type doesn't actually make Reanimated styles work - pass one and StyleSheet.flatten on this line silently throws it away, and Surface is still an RN Animated.View that couldn't apply it anyway. #5078 makes Surface Reanimated, so it's probably worth waiting for that.
There was a problem hiding this comment.
fixed now that #5078 has landed 👌 also added a regression test
104ed60 to
98e4a84
Compare
|
This branch has a copy of built docs committed to id with 900+ files, please remove it 🙏 And also please resolve the conflicts, there've been some changes on main that will also influence this branch, like dropping default test IDs |
671768a to
0c82a80
Compare
0c82a80 to
de46718
Compare
| - `Searchbar`: `style` | ||
| - `Snackbar`: `style` | ||
| - `Surface`: `style` | ||
| - `ToggleButton`: `style` |
There was a problem hiding this comment.
Is the migration guide an older copy of the file? I'm afraid it puts ToggleButton back, drops the removal note for it, and rewrites the whole Test IDs section.
There was a problem hiding this comment.
yep, an older copy slipped in. fixed it
| return ( | ||
| <Reanimated.View | ||
| style={outerStyle} | ||
| testID={testID ? `${testID}-wrapper` : undefined} |
There was a problem hiding this comment.
Have you seen that main dropped every -wrapper / -icon test ID and added searchTestID,
clearTestID and trailingTestID in their place? I'm afraid this brings the old ones back and removes the three props.
| * Icon name for the right trailering icon button. | ||
| * Works only when `mode` is set to "contained". It won't be displayed if `loading` is set to `true`. | ||
| */ | ||
| traileringIcon?: IconSource; |
There was a problem hiding this comment.
Did the rename get lost in the folder move? traileringIcon and similar were corrected to trailingIcon on main, and this brings the old spellings back along with the example, the tests and the migration entry.
| testID={testID ? `${testID}-wrapper` : undefined} | ||
| > | ||
| <Reanimated.View | ||
| style={applyFocusMargin ? containedMarginStyle : null} |
There was a problem hiding this comment.
Have you tried <Searchbar style={{ height: 80 }} />? I'm afraid the bar stays its default height - the height lands on the outer wrapper and this middle view never grows. Might styles.fillWrapper here cover it?
<Reanimated.View
- style={applyFocusMargin ? containedMarginStyle : null}
+ style={[
+ shouldFillWrapper && styles.fillWrapper,
+ applyFocusMargin ? containedMarginStyle : null,
+ ]}
>There was a problem hiding this comment.
good catch 👍 added styles.fillWrapper to the middle view
|
Review by Claude iOS
Steps: open the Searchbar example, scroll to "With results", type What happens: the results surface spans the full screen width and its corners touch the screen edges. The contained bar above it has 24dp side margins, so the two do not line up. What should happen: the results container lines up with the bar, as in the MD3 docked search view. (via Claude, verify if it makes sense) Android
Steps: open the Searchbar example, scroll to "With results", type What happens: the bar is focused with 12dp side margins, and the results surface below it has none. What should happen: the results container lines up with the bar. (via Claude, verify if it makes sense) Web
Steps: open What happens: same as on native. The results surface is full width and the bar has 12dp side margins while focused. What should happen: the results container lines up with the bar. (via Claude, verify if it makes sense) A click on the outer part of the clear button does not clear the text Steps: focus the first bar under "Contained mode", type some text, then press the clear button within about 12px of its outer edge and release after about 100ms. What happens: on mouse down the button takes focus and the input blurs. The bar animates back from 12dp to 24dp margins, so the clear button moves 12px inward while the button is held. The release lands outside the button and the text stays. Pressing Tab from the input to the clear button also collapses the bar while focus is still inside it. What should happen: the bar stays expanded while focus is inside it, and the clear button clears the text wherever it is clicked. (via Claude, verify if it makes sense) |
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comments. (via Claude, verify if it makes sense)
| collect(style); | ||
|
|
||
| return [StyleSheet.flatten(staticStyles), animatedStyles]; |
There was a problem hiding this comment.
This adds StyleSheet.flatten, and isAnimatedStyle (line 96) finds Reanimated styles by checking for viewDescriptors, which is a Reanimated internal and can change in any release. Inspecting the style prop like this is fragile. For example, every animated entry moves to the outer wrapper, so an animated backgroundColor or borderColor ends up behind the Surface and has no visible effect.
The split exists so a custom horizontal margin can beat the animated one, because a useAnimatedStyle value always wins over static styles. If the focus margin is a plain style placed before style in the array (see the comment on the focus animation), normal style order lets the consumer override it. Then the whole style can go to the Surface as an array, and splitAnimatedStyles, HORIZONTAL_MARGIN_KEYS, OUTER_LAYOUT_STYLE_KEYS and the two wrapper views can go away. (via Claude, verify if it makes sense)
| // M3 Expressive focus transition: the contained bar grows wider on focus as | ||
| // its horizontal margin shrinks from `marginUnfocused` to `marginFocused`. | ||
| const focusMargin = useSharedValue<number>(SearchbarTokens.marginUnfocused); | ||
|
|
||
| const growSpring = React.useMemo( | ||
| () => ({ | ||
| ...toRawSpring(theme.motion.spring.fast.spatial), | ||
| reduceMotion: reanimatedReduceMotion, | ||
| }), | ||
| [theme.motion.spring.fast.spatial, reanimatedReduceMotion] | ||
| ); | ||
| const shrinkSpring = React.useMemo( | ||
| () => ({ | ||
| ...toRawSpring(theme.motion.spring.default.spatial), | ||
| reduceMotion: reanimatedReduceMotion, | ||
| }), | ||
| [theme.motion.spring.default.spatial, reanimatedReduceMotion] | ||
| ); |
There was a problem hiding this comment.
The focus change is a transition between two states, so it should use Reanimated CSS transitions like Badge and Checkbox do, and not a shared value with withSpring. Springs on shared values fit gesture-driven or scroll-driven motion, which this is not. For example:
const [focused, setFocused] = React.useState(false);
const focusMarginStyle: CSSStyle<ViewStyle> = {
marginHorizontal: focused
? SearchbarTokens.marginFocused
: SearchbarTokens.marginUnfocused,
transitionProperty: ['marginHorizontal'],
transitionDuration: reduceMotion ? 0 : theme.motion.duration.medium2,
transitionTimingFunction: cubicBezier(...theme.motion.easing.standard),
};With [styles.container, isContained && focusMarginStyle, style] on the Surface, a consumer marginHorizontal, marginLeft or marginStart overrides it, and margin: 4 keeps the animated side margins. That also fixes the surprise in the migration guide where margin turns the animation off. I did not check that Reanimated transitions the marginHorizontal shorthand on web, so that needs a quick check. (via Claude, verify if it makes sense)
| const handleBlur: NonNullable<TextInputProps['onBlur']> = (e) => { | ||
| focusMargin.value = withSpring( | ||
| SearchbarTokens.marginUnfocused, | ||
| shrinkSpring | ||
| ); | ||
| onBlur?.(e); | ||
| }; |
There was a problem hiding this comment.
On web, a press on the clear button or the trailing icon moves focus from the input to that button. The input blurs, so the bar starts to shrink while the mouse button is still down and the clear button moves 12px inward. A click near the outer edge of the clear button then ends outside it and the text is not cleared. Tab from the input to the clear button also collapses the bar while focus is still inside it. On iOS and Android a tap does not blur the input, so the bar stays expanded there.
Could the bar stay expanded while focus is anywhere inside it? On web, the blur event's relatedTarget tells you whether focus moved to a button inside the bar. Steps to reproduce are in the screenshot comment above. (via Claude, verify if it makes sense)
| * - `contained` - the recommended M3 Expressive style: a rounded, elevated | ||
| * bar whose horizontal margins animate from 24dp down to 12dp on focus |
There was a problem hiding this comment.
The default elevation is 0, so a contained bar is not elevated by default.
| * - `contained` - the recommended M3 Expressive style: a rounded, elevated | |
| * bar whose horizontal margins animate from 24dp down to 12dp on focus | |
| * - `contained` - the recommended M3 Expressive style: a rounded bar | |
| * whose horizontal margins animate from 24dp down to 12dp on focus | |
| ``` (via Claude, verify if it makes sense) |
| const styles = StyleSheet.create({ | ||
| container: { | ||
| width: '100%', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Searchbar.Results spans the full width, while the contained bar above it has 24dp side margins (12dp while focused). In the example and in the usage snippet in this file, the results surface is wider than the bar and its corners touch the screen edges on all three platforms. In the MD3 docked search view the results line up with the bar.
Could Results use the bar's horizontal margin token (marginFocused, since results usually show while the user types), with style able to override it like on the bar? (via Claude, verify if it makes sense)
| describe('getSearchbarColors', () => { | ||
| it('resolves the MD3 color roles, with input/placeholder in spec order', () => { | ||
| const theme = LightTheme; | ||
| expect(getSearchbarColors(theme)).toEqual({ | ||
| containerColor: theme.colors.surfaceContainerHigh, | ||
| inputColor: theme.colors.onSurface, | ||
| placeholderColor: theme.colors.onSurfaceVariant, | ||
| leadingIconColor: theme.colors.onSurface, | ||
| trailingIconColor: theme.colors.onSurfaceVariant, | ||
| cursorColor: theme.colors.primary, | ||
| dividerColor: theme.colors.outline, | ||
| resultsContainerColor: theme.colors.surfaceContainerHigh, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests call internal helpers directly and repeat the token table from tokens.ts line by line, so they fail on any token change without checking what users see. The project test rules ask for direct helper tests only when the logic is complex, and these helpers are a lookup and a Platform.select. The clear icon color test in Searchbar.test.tsx already checks a color role through rendered output. Could the input and placeholder colors be checked the same way there, and this file be removed? (via Claude, verify if it makes sense)



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