More nativewind friendly#1037
Conversation
There was a problem hiding this comment.
Code Review
This pull request integrates NativeWind (v5) into the UI library, adding className prop support to various core components—including DeckSwiperCard, LoadingIndicator, LottieAnimation, Markdown, AudioPlayer, PinInput, LinearProgress, Swiper, TableRow, Timer, and WebView—to enable Tailwind CSS styling. It also configures the example application with Tailwind CSS and PostCSS, introducing a comprehensive NativeWindExample screen. The review feedback highlights a recurring fragile pattern where default styles (such as background colors, borders, widths, and text styles) are conditionally omitted when a className is present. This can cause components to lose essential default styles if a developer passes an unrelated utility class (e.g., for margins or padding). Additionally, the feedback points out a potential double-styling bug in the Markdown component and a missing style prop forwarding in the PinInput component.
| !className && { | ||
| backgroundColor: theme.colors.background.base, | ||
| borderColor: theme.colors.border.base, | ||
| }, |
There was a problem hiding this comment.
Conditionally omitting the default background and border styles when className is present is highly fragile. If a developer passes a className for an unrelated style (such as padding or margins, e.g., className="p-4"), the component will completely lose its default background and border colors. Always include the default styles in the style array; NativeWind's compiled styles from className will naturally override these defaults because they are merged/appended at the end of the style array.
| !className && { | |
| backgroundColor: theme.colors.background.base, | |
| borderColor: theme.colors.border.base, | |
| }, | |
| { | |
| backgroundColor: theme.colors.background.base, | |
| borderColor: theme.colors.border.base, | |
| }, |
|
|
||
| const textStyles = { | ||
| color, | ||
| color: color ?? (className ? undefined : theme.colors.text.strong), |
There was a problem hiding this comment.
Using className to conditionally omit the default text color is fragile. If className is provided for layout or spacing (e.g., className="mt-2"), the text color will become undefined and fall back to the system default instead of theme.colors.text.strong. It is safer to always provide the default color and let NativeWind's style merging handle overrides.
| color: color ?? (className ? undefined : theme.colors.text.strong), | |
| color: color ?? theme.colors.text.strong, |
| !className && { | ||
| backgroundColor: theme.colors.background.base, | ||
| borderColor: theme.colors.border.base, | ||
| }, |
There was a problem hiding this comment.
Omitting the default background and border styles when className is present will cause the player to lose its background and border if a developer passes a className for an unrelated property (like margins or width). Always include the default styles so they can be overridden gracefully by NativeWind.
| !className && { | |
| backgroundColor: theme.colors.background.base, | |
| borderColor: theme.colors.border.base, | |
| }, | |
| { | |
| backgroundColor: theme.colors.background.base, | |
| borderColor: theme.colors.border.base, | |
| }, |
| isTableHeader && !className | ||
| ? { backgroundColor: theme.colors.branding.primary } | ||
| : {}, |
There was a problem hiding this comment.
Omitting the default header background color when className is present means that any table header with a custom class (e.g., for border or margin) will lose its primary branding background color. It is better to always apply the default background and let NativeWind override it if a background class is explicitly provided.
| isTableHeader && !className | |
| ? { backgroundColor: theme.colors.branding.primary } | |
| : {}, | |
| isTableHeader | |
| ? { backgroundColor: theme.colors.branding.primary } | |
| : {}, |
| <Text {...rest} style={[styles.defaultTimerStyle, style]}> | ||
| <Text | ||
| {...rest} | ||
| style={[!className && styles.defaultTimerStyle, style]} |
There was a problem hiding this comment.
Completely omitting styles.defaultTimerStyle when className is present will strip all default text styling (like font size, alignment, etc.) even if the className only specifies something unrelated like text color or margin. It is safer to always include the default styles in the array.
| style={[!className && styles.defaultTimerStyle, style]} | |
| style={[styles.defaultTimerStyle, style]} |
| { | ||
| height: maxThickness, | ||
| width: "100%", | ||
| ...(className ? {} : { width: "100%" }), |
There was a problem hiding this comment.
Omitting the default width: "100%" when className is present will cause the progress bar to collapse to 0 width if the className does not explicitly define a width (e.g., if it only defines a margin like className="mt-3"). It is safer to always default to width: "100%" and let the className override it if a width is specified.
| ...(className ? {} : { width: "100%" }), | |
| width: "100%", |
| const bodyStyle = StyleSheet.flatten(style); | ||
| const { textStyles } = extractStyles(bodyStyle); | ||
| const { viewStyles, textStyles } = extractStyles(bodyStyle); | ||
|
|
||
| return ( | ||
| //'body' style applies to all markdown components | ||
| //@ts-ignore TS does not like the type of this named style for some reason | ||
| <MarkdownComponent | ||
| style={{ body: bodyStyle }} | ||
| rules={{ | ||
| // By default, strong does not work with custom fonts on iOS. This addresses the issue | ||
| strong: (node) => ( | ||
| <Text | ||
| key={node.key} | ||
| style={{ | ||
| ...textStyles, | ||
| fontWeight: "bold", | ||
| }} | ||
| > | ||
| {node.children[0].content} | ||
| </Text> | ||
| ), | ||
| }} | ||
| <View | ||
| style={viewStyles} | ||
| // @ts-ignore | ||
| className={className} | ||
| > | ||
| {text} | ||
| </MarkdownComponent> | ||
| {/*//'body' style applies to all markdown components*/} | ||
| {/*@ts-ignore TS does not like the type of this named style for some reason*/} | ||
| <MarkdownComponent | ||
| style={{ body: bodyStyle }} |
There was a problem hiding this comment.
By passing bodyStyle (which contains both view and text styles) to MarkdownComponent while also applying viewStyles to the outer wrapper View, any container styles (like padding, margin, backgroundColor, or border) will be applied twice. This will cause layout issues and visual bugs (such as doubled margins/padding). To fix this, you should pass only textStyles (or a style object with view styles omitted) to the MarkdownComponent's body style, since the outer View already handles the container/view styles.
| const bodyStyle = StyleSheet.flatten(style); | |
| const { textStyles } = extractStyles(bodyStyle); | |
| const { viewStyles, textStyles } = extractStyles(bodyStyle); | |
| return ( | |
| //'body' style applies to all markdown components | |
| //@ts-ignore TS does not like the type of this named style for some reason | |
| <MarkdownComponent | |
| style={{ body: bodyStyle }} | |
| rules={{ | |
| // By default, strong does not work with custom fonts on iOS. This addresses the issue | |
| strong: (node) => ( | |
| <Text | |
| key={node.key} | |
| style={{ | |
| ...textStyles, | |
| fontWeight: "bold", | |
| }} | |
| > | |
| {node.children[0].content} | |
| </Text> | |
| ), | |
| }} | |
| <View | |
| style={viewStyles} | |
| // @ts-ignore | |
| className={className} | |
| > | |
| {text} | |
| </MarkdownComponent> | |
| {/*//'body' style applies to all markdown components*/} | |
| {/*@ts-ignore TS does not like the type of this named style for some reason*/} | |
| <MarkdownComponent | |
| style={{ body: bodyStyle }} | |
| const bodyStyle = StyleSheet.flatten(style); | |
| const { viewStyles, textStyles } = extractStyles(bodyStyle); | |
| return ( | |
| <View | |
| style={viewStyles} | |
| // @ts-ignore | |
| className={className} | |
| > | |
| {/*//'body' style applies to all markdown components*/} | |
| {/*@ts-ignore TS does not like the type of this named style for some reason*/} | |
| <MarkdownComponent | |
| style={{ body: textStyles }} |
| // @ts-ignore | ||
| <View className={className}> |
There was a problem hiding this comment.
The style prop is destructured from the component's props but is not passed to the wrapper View. To ensure that both inline styles and className work correctly and consistently with other components, you should pass style={style} to the wrapper View.
| // @ts-ignore | |
| <View className={className}> | |
| // @ts-ignore | |
| <View style={style} className={className}> |
No description provided.