[POC] Porting Icon component from gluestack-ui V2 #341
Conversation
With this new component, you can import Lucide icons on demand,
like this:
<Icon as={Github} />
References:
- https://gluestack.io/ui/docs/components/icon
- https://lucide.dev
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes add new UI elements and underlying icon creation functionality. Two new buttons for Android and Apple are introduced to the ButtonScreen component. A new Icon component is implemented to support different size variants and styling through libraries like class-variance-authority and nativewind. Additionally, a suite of icon utilities and components is provided for both native and web environments. These include functions and types for creating icons with SVG elements and re-exports that streamline the usage of icon components throughout the application. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as ButtonScreen
participant Ic as Icon Component
participant Cr as createIcon Function
participant P as PrimitiveIcon Component
U->>B: Interacts with ButtonScreen
B->>Ic: Renders Android/Apple button with icon
Ic->>Cr: Calls createIcon for icon creation
Cr->>P: Delegates to PrimitiveIcon for SVG rendering
P-->>Cr: Returns rendered SVG icon
Cr-->>Ic: Provides icon element
Ic-->>B: Renders button with icon
B-->>U: Displays updated interface
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
apps/showcase/lib/icon.tsx (5)
13-27: Consider providing numeric fallback
The usage of cva for handling multiple size variants is robust. However, consider providing a numeric fallback for size when no variant is provided. This can further handle dynamic or user-based inputs more gracefully.
29-40: Clarify bridging of native styles
Mapping style props like height or width to the className could be confusing for maintainers. Consider adding documentation or an inline comment explaining how cssInterop merges native style attributes and class-based styling.
42-44: Refine the naming of IIConProps
While the “I” prefix denotes an interface, “IIConProps” might be unintuitive. Consider a name like “IconProps” or “IconComponentProps” for clarity.
46-55: Size handling logic
The approach of conditionally rendering based on numeric vs. variant size is solid. Ensure consistent behavior when only one dimension (width or height) is specified, to avoid distorted icons.
57-86: Memoizing createIconUI
The dynamic creation of a forward-ref icon component is a powerful pattern. However, repeatedly invoking createIconUI might impact performance if done frequently. Consider memoizing or providing a stable reference to the returned component.Would you like assistance in implementing a memoized solution or opening a follow-up issue?
apps/showcase/lib/rnr-icon/primitiveIcon/index.tsx (2)
16-16: Consider using a more specific type for style prop.Replace
anywith a proper type definition to improve type safety.- style?: any; + style?: React.ComponentProps<typeof Svg>['style'];
44-52: Simplify color props handling.The current implementation has multiple spread operations which could be simplified.
- let colorProps = {}; - if (fill) { - colorProps = { ...colorProps, fill: fill }; - } - if (stroke !== 'currentColor') { - colorProps = { ...colorProps, stroke: stroke }; - } else if (stroke === 'currentColor' && color !== undefined) { - colorProps = { ...colorProps, stroke: color }; - } + const colorProps = { + ...(fill && { fill }), + ...(stroke !== 'currentColor' + ? { stroke } + : color !== undefined && { stroke: color }), + };apps/showcase/lib/rnr-icon/primitiveIcon/index.web.tsx (1)
35-51: Consider extracting shared logic to reduce duplication.The size and color handling logic is identical between web and native implementations.
Consider creating a shared utility file for common logic:
// lib/rnr-icon/utils.ts export const getSizeProps = (size?: number | string, height?: number | string, width?: number | string) => { if (size) return { size }; if (height && width) return { height, width }; if (height) return { height }; if (width) return { width }; return {}; }; export const getColorProps = (fill?: string, stroke?: string, color?: string) => { return { ...(fill && { fill }), ...(stroke !== 'currentColor' ? { stroke } : color !== undefined && { stroke: color }), }; };apps/showcase/lib/rnr-icon/createIcon/index.tsx (1)
93-93: Remove commented out code.Clean up the commented line as it's not providing value.
- // sizeProps = { ...sizeProps, fontSize: resolvedProps?.size };apps/showcase/lib/rnr-icon/createIcon/index.web.tsx (3)
30-30: Consider removing empty interface if no additional props are needed.The empty interface extension might indicate unnecessary abstraction. If no additional props are needed beyond
ViewProps, consider usingViewPropsdirectly.
89-98: Clean up size handling logic.There's commented out code and potentially unused variables:
- Empty
sizeStyleobject is spread but never populated- Commented out size handling code
- let sizeProps = {}; - let sizeStyle = {}; + const sizeProps: { fontSize?: number } = {}; if (type === 'font') { if (resolvedProps.sx) { sizeProps = { ...sizeProps, fontSize: resolvedProps?.sx?.h }; } - if (resolvedProps.size) { - // sizeProps = { ...sizeProps, fontSize: resolvedProps?.size }; - } }
66-69: Optimize object spread operations.The code creates new objects in every render through spread operations. Consider memoizing these objects or moving the logic outside the render function.
+ const finalProps = useMemo(() => ({ + ...initialProps, + ...props, + }), [props]); + + const colorProps = useMemo(() => { + const result = {}; + if (color) { + result.color = color; + } + if (stroke) { + result.stroke = stroke; + } + return result; + }, [color, stroke]);Don't forget to import
useMemofrom React:-import React, { forwardRef } from 'react'; +import React, { forwardRef, useMemo } from 'react';Also applies to: 82-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/showcase/app/button.tsx(2 hunks)apps/showcase/lib/icon.tsx(1 hunks)apps/showcase/lib/rnr-icon/createIcon/index.tsx(1 hunks)apps/showcase/lib/rnr-icon/createIcon/index.web.tsx(1 hunks)apps/showcase/lib/rnr-icon/index.tsx(1 hunks)apps/showcase/lib/rnr-icon/index.web.tsx(1 hunks)apps/showcase/lib/rnr-icon/primitiveIcon/index.tsx(1 hunks)apps/showcase/lib/rnr-icon/primitiveIcon/index.web.tsx(1 hunks)
🔇 Additional comments (7)
apps/showcase/lib/icon.tsx (3)
1-5: No immediate concerns with the import statements
Everything seems correct here with the import ordering and usage.
6-11: UIIcon creation
The usage of createIcon is straightforward and consistent. Great job utilizing ForwardRefExoticComponent for better type safety.
87-87: Export re-aliased createIcon
Re-exporting the createIcon function under a consistent name fosters clarity. Nicely done.apps/showcase/lib/rnr-icon/index.web.tsx (1)
1-3: Clean re-exports
Re-exporting the underlying modules in a single index fosters better discoverability and simpler imports.apps/showcase/lib/rnr-icon/index.tsx (1)
1-3: Consistent re-exports
Re-exporting key icon utilities and components from a central index is a best practice. This consolidates your library’s public API.apps/showcase/app/button.tsx (1)
34-41: LGTM! Clean implementation of icon buttons.The new buttons follow consistent patterns with proper:
- Layout using flex-row
- Icon sizing and spacing
- Color theming integration
apps/showcase/lib/rnr-icon/createIcon/index.tsx (1)
88-95: Complete the font type icon handling.The font type handling is incomplete with partial implementation.
Please verify:
- Is font type support needed for this POC?
- If yes, should we implement the size handling for font type icons?
| const accessClassName = (style: any) => { | ||
| const styleObject = Array.isArray(style) ? style[0] : style; | ||
| const keys = Object.keys(styleObject); | ||
| return styleObject[keys[1]]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of accessClassName utility.
The current implementation makes assumptions about the style object structure and could fail silently.
const accessClassName = (style: any) => {
+ if (!style) return undefined;
const styleObject = Array.isArray(style) ? style[0] : style;
+ if (typeof styleObject !== 'object') return undefined;
const keys = Object.keys(styleObject);
+ if (keys.length < 2) return undefined;
return styleObject[keys[1]];
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const accessClassName = (style: any) => { | |
| const styleObject = Array.isArray(style) ? style[0] : style; | |
| const keys = Object.keys(styleObject); | |
| return styleObject[keys[1]]; | |
| }; | |
| const accessClassName = (style: any) => { | |
| if (!style) return undefined; | |
| const styleObject = Array.isArray(style) ? style[0] : style; | |
| if (typeof styleObject !== 'object') return undefined; | |
| const keys = Object.keys(styleObject); | |
| if (keys.length < 2) return undefined; | |
| return styleObject[keys[1]]; | |
| }; |
| defaultProps?: any; | ||
| type?: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by avoiding any types.
The defaultProps and type properties are typed as any, which reduces type safety. Consider defining specific types based on their expected values.
- defaultProps?: any;
- type?: any;
+ defaultProps?: Partial<IIconProps>;
+ type?: 'svg' | 'font';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaultProps?: any; | |
| type?: any; | |
| defaultProps?: Partial<IIconProps>; | |
| type?: 'svg' | 'font'; |
| const ChildPath = ({ element, fill, stroke: pathStroke }: any) => { | ||
| const pathStrokeColor = pathStroke || ''; | ||
| const fillColor = fill || ''; | ||
|
|
||
| if (!element) { | ||
| return null; | ||
| } | ||
|
|
||
| if (element.type === React.Fragment) { | ||
| return element; | ||
| } | ||
|
|
||
| return React.cloneElement(element, { | ||
| fill: fillColor ? fillColor : 'currentColor', | ||
| stroke: pathStrokeColor, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and prop validation in ChildPath component.
The component has several type-safety concerns:
- Props are typed as
any - No prop types validation
- Missing proper TypeScript types for element prop
-const ChildPath = ({ element, fill, stroke: pathStroke }: any) => {
+interface ChildPathProps {
+ element: React.ReactElement | null;
+ fill?: string;
+ stroke?: string;
+}
+
+const ChildPath = ({ element, fill, stroke: pathStroke }: ChildPathProps) => {Also, consider adding prop-types validation if you're targeting environments where TypeScript types aren't available at runtime.
| d, | ||
| ...initialProps | ||
| }: { Root: React.ComponentType<IconProps> } & CreateIconOptions) { | ||
| const IconTemp = forwardRef((props: any, ref?: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in IconTemp component.
The component props are typed as any. Consider creating a proper interface for the props.
-const IconTemp = forwardRef((props: any, ref?: any) => {
+interface IconTempProps extends IIconProps {
+ stroke?: string;
+ color?: string;
+ role?: string;
+ type?: 'svg' | 'font';
+ sx?: { h?: number };
+ size?: number;
+}
+const IconTemp = forwardRef<unknown, IconTempProps>((props, ref) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const IconTemp = forwardRef((props: any, ref?: any) => { | |
| interface IconTempProps extends IIconProps { | |
| stroke?: string; | |
| color?: string; | |
| role?: string; | |
| type?: 'svg' | 'font'; | |
| sx?: { h?: number }; | |
| size?: number; | |
| } | |
| const IconTemp = forwardRef<unknown, IconTempProps>((props, ref) => { | |
| // existing implementation continues... |
|
Interesting. I'm pretty sure it means that this could also work: import type { LucideIcon, LucideProps } from 'lucide-react-native';
import { cssInterop } from 'nativewind';
function Icon({ as: AsProp, ...props}: LucideIconProps & { as: LucideIcon }){
return <AsProp {...props} />
}
cssInterop(Icon, {
className: {
target: 'style',
nativeStyleToProp: {
color: true,
opacity: true,
},
},
});
export { Icon } |
|
Hi @mrzachnugent, There is just one little typo, Thank you 👍 |
Description:
react-native-reusablesWith this new component, you can import Lucide icons on demand, like this:
Check the code here, for a more complete example
It's a strict port of gluestack-ui V2 component, the coding style and the default CSS should be updating according to
react-native-reusablesstandardsReferences:
Related issue: #202
Tested Platforms:
Affected Apps/Packages:
Screenshots:
Notes:
Many thanks to @mrzachnugent to bring us this wonderful library 👍
Summary by CodeRabbit