♻️ Refacto link (breaking) #253
Conversation
|
Warning Rate limit exceeded@FelixLgr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThe pull request introduces significant modifications to the Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (7)
packages/react/components/link/Link.stories.tsx (1)
20-20: Consider adding a comment to explain markup usageThe story demonstrates the new
markupprop usage, but it would be helpful to add a comment explaining when to use 'span' vs default markup.+ {/* Use markup='span' for inline links within text content */} Je suis dans un paragraphe et ceci est un <Link markup='span'>lien standard</Link>packages/react/components/link/LinkProps.ts (1)
11-11: LGTM! Good architectural improvement.The addition of the
markupprop and removal of specialized props likeiconNameandinlinemakes the Link component more focused and flexible. This change follows the single responsibility principle by delegating icon rendering to the consumer.packages/react/components/link/test/link.test.tsx (1)
32-34: Consider adding test cases for different markup types.While the test covers the basic markup='a' case, consider adding test cases for other common element types (button, div, etc.) to ensure the markup prop works correctly with different elements.
Example test case to add:
test('should render with different markup elements', () => { const { container } = render( <> <Link markup="button">Button Link</Link> <Link markup="div">Div Link</Link> </> ); expect(container.querySelector('button')).toBeInTheDocument(); expect(container.querySelector('div')).toBeInTheDocument(); });packages/react/components/link/Link.tsx (2)
4-4: Remove unnecessary space in import statementThe import statement contains extra spaces between curly braces.
-import { is } from '@/services/classify' +import { is } from '@/services/classify'
40-51: Component rendering has been effectively simplifiedThe new rendering approach with
LinkComponentis more flexible and maintainable:
- Proper spreading of props
- Clean conditional handling of blank target
- Appropriate aria-label implementation
However, consider adding prop-types or TypeScript validation for the
markupprop to ensure only valid HTML elements or components are passed.+const validMarkups = ['a', 'button', 'div', 'span'] as const +type ValidMarkup = typeof validMarkups[number] + const Link = ({ children, className, accessibilityLabel, - markup: LinkComponent = 'a', + markup: LinkComponent = 'a' as ValidMarkup, inverted, blank, ...others -}: LinkProps): JSX.Element => { +}: LinkProps & { markup?: ValidMarkup | React.ComponentType }): JSX.Element => {packages/react/components/link/Link.native.tsx (2)
24-28: Review hardcoded padding and margin valuesThe removal of dynamic padding based on
inlineprop has led to hardcoded values. Consider:
- Using design tokens for these values
- Making these values configurable through theme
+const LINK_STYLES = { + padding: 8, + marginTop: 0, + marginBottom: -10, + paddingHorizontal: 0, +} as const const styles = StyleSheet.create({ container: { - padding: 8, - marginTop: 0, - marginBottom: -10, - paddingLeft: 0, - paddingRight: 0, + ...LINK_STYLES, }, androidContainer: { - paddingLeft: 0, - paddingRight: 0, + paddingHorizontal: LINK_STYLES.paddingHorizontal, },Also applies to: 31-32
90-96: Text component implementation is clean but could be more accessibleThe Text component implementation is clean, but consider enhancing accessibility:
<Text accessibilityLabel={accessibilityLabel} + accessibilityRole="link" + accessible={true} style={Platform.OS === 'android' ? [styles.androidLink] : [styles.link]} {...others} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/react-template/screens/Link.tsx(2 hunks)packages/react/components/link/Link.native.tsx(2 hunks)packages/react/components/link/Link.stories.tsx(1 hunks)packages/react/components/link/Link.tsx(2 hunks)packages/react/components/link/LinkProps.ts(1 hunks)packages/react/components/link/test/link.test.tsx(1 hunks)
🔇 Additional comments (7)
packages/react/components/link/Link.stories.tsx (2)
31-31: LGTM! Good demonstration of markup prop usage
The story correctly demonstrates how to use the markup prop for inline links.
44-51: LGTM! Clear demonstration of inverted prop
The story effectively shows how to use the inverted prop with appropriate background context.
examples/react-template/screens/Link.tsx (2)
2-3: LGTM! Clean import organization.
Good separation of concerns by splitting the imports between core components and icon-specific imports.
19-22: Verify icon positioning matches previous behavior.
The icon is now explicitly composed within the Link component. While this provides more flexibility, ensure that the visual presentation (spacing, alignment) matches the previous implementation where iconName was used.
packages/react/components/link/Link.tsx (2)
28-35: Props interface has been effectively simplified
The removal of iconName and addition of markup prop with a default value of 'a' improves the component's flexibility while reducing complexity. The destructuring is clean and well-organized.
38-38: Verify the impact of removing iconName from classes
The class construction has been simplified, but we should verify that this doesn't break existing styles that might depend on icon-related classes.
✅ Verification successful
Removal of icon-related classes is safe to proceed
Based on the comprehensive review of the changes and related files:
- The
iconNameprop and related code has been completely removed from the Link component - The test files don't contain any icon-specific test cases that would break
- The stories don't showcase any icon-related use cases
- The LinkProps interface has been cleaned up to remove icon-related props
- No CSS/SCSS files were found that depend on the icon-related classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any CSS/SCSS files that might reference icon-related classes
rg -t css -t scss 'link.*icon|icon.*link'
Length of output: 72
Script:
#!/bin/bash
# Search for any CSS/SCSS files that might reference icon-related classes
# Using fd to find style files first, then grep through them
fd -e css -e scss -e sass | xargs rg 'link.*icon|icon.*link'
# Also check for styled-components usage with icon in Link component
ast-grep --pattern 'styled.$_`
$_icon$_
`'
Length of output: 109
Script:
#!/bin/bash
# Let's check the history of the Link component to understand the icon-related changes
git show HEAD -- packages/react/components/link/Link.tsx
# Also search for any tests or stories that might show icon usage
fd -e test.tsx -e test.ts -e stories.tsx -e stories.ts | xargs rg -l 'Link.*icon|icon.*Link'
# Check for any styled components or CSS modules directly related to Link
fd -g '*link*' -t f packages/react/components/link/
Length of output: 3982
Script:
#!/bin/bash
# Let's check the test files and stories to see if there were any icon-related test cases
cat packages/react/components/link/test/link.test.tsx
cat packages/react/components/link/Link.stories.tsx
# Also check LinkProps to understand the complete removal of icon-related props
cat packages/react/components/link/LinkProps.ts
Length of output: 3235
packages/react/components/link/Link.native.tsx (1)
17-17: Props interface aligns with web version
The props interface has been effectively simplified to match the web version, maintaining consistency across platforms.
Summary by CodeRabbit
New Features
markupprop for theLinkcomponent, allowing flexible rendering options.Bug Fixes
markupprop.Documentation
LinkPropsinterface and component structure.Refactor
Linkcomponent by removing unnecessary props and streamlining rendering logic.