Feat: Add 'onRightIconClick' prop on TextInput #1492
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a new optional property, Changes
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/ui/src/components/TextInput/TextInput.tsx (3 hunks)
- packages/ui/src/components/TextInput/theme.ts (1 hunks)
Additional comments not posted (4)
packages/ui/src/components/TextInput/theme.ts (1)
15-15: Removal ofpointer-events-noneenables click events on the right iconThe removal of the
pointer-events-noneclass from therightIcon.baseproperty aligns with the PR objective of adding anonRightIconClickprop to the TextInput component. This change allows click events to be captured on the right icon, which is necessary for implementing features like toggling password visibility.However, there are a few considerations:
- Ensure that this change is accompanied by appropriate updates in the component's logic to handle the click events.
- Consider adding a cursor style (e.g.,
cursor-pointer) to visually indicate that the icon is clickable.- Verify that this change doesn't introduce any unintended side effects for existing uses of the TextInput component.
To ensure this change is properly implemented and doesn't cause regressions, please run the following verification script:
Verification successful
Conditional application of
pointer-events-noneis correctly implementedThe
pointer-events-noneclass is now conditionally applied based on the presence of theonRightIconClickprop. This ensures that click events on the right icon are only enabled when anonRightIconClickhandler is provided, aligning with the PR's objective.No unintended uses of
pointer-events-noneremain whenonRightIconClickis defined.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of onRightIconClick in TextInput component # Test 1: Check if the TextInput component has been updated to handle onRightIconClick echo "Checking TextInput component for onRightIconClick implementation:" ast-grep --lang typescript --pattern 'interface TextInputProps { $$$ onRightIconClick?: $_ $$$ }' # Test 2: Look for examples or tests demonstrating the use of onRightIconClick echo "Searching for examples or tests using onRightIconClick:" rg --type typescript "onRightIconClick" -C 5 # Test 3: Ensure no unintended uses of pointer-events-none remain for the right icon echo "Checking for any remaining pointer-events-none on right icons:" rg --type typescript "rightIcon.*pointer-events-none"Length of output: 673
Script:
#!/bin/bash # Description: Re-verify the implementation of onRightIconClick in TextInput component with corrected file types # Test 1: Check if the TextInput component has been updated to handle onRightIconClick echo "Checking TextInput component for onRightIconClick implementation:" ast-grep --lang typescript --pattern 'interface TextInputProps { $$$ onRightIconClick?: $_ $$$ }' # Test 2: Look for examples or tests demonstrating the use of onRightIconClick echo "Searching for examples or tests using onRightIconClick:" rg "onRightIconClick" -C 5 --type ts # Test 3: Ensure no unintended uses of pointer-events-none remain for the right icon echo "Checking for any remaining pointer-events-none on right icons:" rg "rightIcon.*pointer-events-none" --type tsLength of output: 3566
packages/ui/src/components/TextInput/TextInput.tsx (3)
49-49: LGTM: New proponRightIconClickadded correctlyThe addition of the
onRightIconClickprop to theTextInputPropsinterface is implemented correctly. It's defined as an optional function returning void, which is appropriate for a click event handler. This change aligns well with the PR objectives of enabling password visibility toggle functionality.
64-64: LGTM:onRightIconClickprop correctly destructuredThe
onRightIconClickprop is correctly added to the destructured props in the component. This makes the new prop available for use within the component, which is necessary for implementing the desired functionality.
Line range hint
1-114: Overall assessment: Well-implemented feature with minor suggestionThe implementation of the
onRightIconClickprop in the TextInput component is well done and aligns perfectly with the PR objectives. The changes are clean, follow existing patterns, and improve the component's functionality without introducing any major issues.Key points:
- The new prop is correctly added to the interface and component.
- The implementation in JSX is sound, with good UX considerations (disabling pointer events when no handler is provided).
- A small suggestion was made to add a null check for extra safety when calling
onRightIconClick.Great job on this feature implementation!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/ui/src/components/TextInput/TextInput.tsx (3 hunks)
🔇 Additional comments (3)
packages/ui/src/components/TextInput/TextInput.tsx (3)
49-49: LGTM: New proponRightIconClickadded correctlyThe addition of the
onRightIconClickprop to theTextInputPropsinterface is well-implemented. It's correctly defined as an optional prop with the appropriate event type, which aligns with the PR objectives and enhances the component's functionality.
64-64: LGTM:onRightIconClickcorrectly destructuredThe
onRightIconClickprop is correctly destructured from the component's props, making it available for use within the component. This change is consistent with the addition of the new prop and follows the existing code structure.
Line range hint
1-114: Overall assessment: Good implementation with minor improvement suggestedThe changes in this file successfully implement the new
onRightIconClickfunctionality for theTextInputcomponent. The new prop is correctly added to the interface, destructured in the component, and integrated into the rendering logic. The implementation aligns well with the PR objectives and follows existing code patterns.A minor improvement was suggested for the
onClickimplementation to ensure consistent behavior and avoid unnecessary function creation. With this small change, the implementation will be even more robust and efficient.Great job on enhancing the
TextInputcomponent's functionality!
| <div | ||
| data-testid="right-icon" | ||
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | ||
| onClick={(e) => onRightIconClick?.(e)} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Improve onClick implementation
The changes to the right icon rendering logic look good overall. The addition of data-testid improves testability, and the conditional class for pointer events enhances UX. However, the onClick implementation could be further improved:
- Always pass the event object to
onRightIconClick. - Use optional chaining to ensure
onRightIconClickexists before calling it.
Here's a suggested improvement:
- onClick={(e) => onRightIconClick?.(e)}
+ onClick={onRightIconClick ? (e) => onRightIconClick(e) : undefined}This change ensures that:
- The event object is always passed when
onRightIconClickis called. onRightIconClickis only set as theonClickhandler if it's defined, avoiding unnecessary function creation.
📝 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.
| <div | |
| data-testid="right-icon" | |
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | |
| onClick={(e) => onRightIconClick?.(e)} | |
| > | |
| <div | |
| data-testid="right-icon" | |
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | |
| onClick={onRightIconClick ? (e) => onRightIconClick(e) : undefined} | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/ui/src/components/TextInput/TextInput.tsx (3 hunks)
🔇 Additional comments (3)
packages/ui/src/components/TextInput/TextInput.tsx (3)
49-49: LGTM: New proponRightIconClickcorrectly addedThe new
onRightIconClickprop is correctly defined as an optional function with the appropriate event type. This addition aligns well with the PR objectives and provides good type safety for users of the component.
64-64: LGTM:onRightIconClickcorrectly destructuredThe
onRightIconClickprop is correctly added to the destructured props, making it available for use within the component. Its placement among other similar props is appropriate.
Line range hint
1-118: Summary: Successfully implementedonRightIconClickwith minor improvement suggestedThe changes in this file successfully implement the new
onRightIconClickfunctionality for the TextInput component, aligning well with the PR objectives. The new prop is correctly defined, destructured, and integrated into the component's logic. The addition ofdata-testidand the conditional class for pointer events are good improvements for testability and UX.A minor improvement was suggested for the
onClickimplementation to enhance type safety and ensure proper event handling. Overall, the changes are well-implemented and ready for merging after addressing the suggested improvement.
| <div | ||
| data-testid="right-icon" | ||
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | ||
| onClick={onRightIconClick} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve onClick implementation for better type safety and event handling
The changes to the right icon rendering logic look good overall. The addition of data-testid improves testability, and the conditional class for pointer events enhances UX. However, the onClick implementation can be further improved:
- Always pass the event object to
onRightIconClick. - Use a more type-safe approach to ensure
onRightIconClickexists before setting it as the handler.
Here's a suggested improvement:
- onClick={onRightIconClick}
+ onClick={onRightIconClick ? (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => onRightIconClick(e) : undefined}This change ensures that:
- The event object is always passed when
onRightIconClickis called. onRightIconClickis only set as theonClickhandler if it's defined, avoiding potential runtime errors.- The types are explicitly defined for better type safety.
📝 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.
| <div | |
| data-testid="right-icon" | |
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | |
| onClick={onRightIconClick} | |
| > | |
| <div | |
| data-testid="right-icon" | |
| className={`${theme.field.rightIcon.base} ${!onRightIconClick ? "pointer-events-none" : ""}`} | |
| onClick={onRightIconClick ? (e: React.MouseEvent<HTMLDivElement, MouseEvent>) => onRightIconClick(e) : undefined} | |
| > |
|
Is there any reason why is this PR not accepted? |
Summary
This PR adds a prop on
TextInputto support onClick for the right icon.This allows users to easily create password inputs with the use of
rightIconprop.Demo
Code Example
Summary by CodeRabbit
New Features
onRightIconClickfor theTextInputcomponent, allowing users to define actions when the right icon is clicked.Bug Fixes
Style
TextInput, enhancing its visual integration and functionality.