fix: disable number change in case of scrolling#4257
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
@Ankit1ml is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a new optional property, Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
packages/components/input/src/use-input.ts (2)
85-88: Improve JSDoc documentation for the new propertyThe documentation for
disableScrollChangeneeds enhancement:
- Fix the typo in "chnage"
- Add type information
- Include example usage
- Document default value
/** - * disable the number chnage on scrolling + * Disables value changes triggered by mouse wheel or touchpad scrolling when the input type is "number". + * This is useful when the input is placed within a scrollable container to prevent accidental value modifications. + * @default false + * @example + * ```tsx + * <Input type="number" disableScrollChange /> + * ``` */ disableScrollChange?: boolean;
399-399: Optimize wheel event handler attachmentThe wheel event handler should only be attached when necessary to optimize performance:
- onWheel: handleScrollingChange, + ...(type === "number" ? { onWheel: handleScrollingChange } : {}),This change ensures the wheel event handler is only attached to number inputs, reducing unnecessary event listeners and potential re-renders.
Also applies to: 416-416
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/components/input/src/use-input.ts(5 hunks)
🔇 Additional comments (1)
packages/components/input/src/use-input.ts (1)
85-88: Add test coverage for the new functionality
To ensure reliability of the disableScrollChange feature, consider adding the following test cases:
- Verify that scrolling is disabled when the prop is true
- Verify that scrolling still works when the prop is false
- Verify behavior with readonly/disabled states
- Verify that non-number inputs are unaffected
| const handleScrollingChange = (e: React.WheelEvent<HTMLInputElement>) => { | ||
| if (disableScrollChange) { | ||
| (e.target as HTMLInputElement).blur(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance scroll handler implementation
The current implementation can be improved for better performance and consistency:
- Only apply the behavior to number inputs
- Respect readonly and disabled states
- Add type safety
- const handleScrollingChange = (e: React.WheelEvent<HTMLInputElement>) => {
- if (disableScrollChange) {
- (e.target as HTMLInputElement).blur();
- }
- };
+ const handleScrollingChange = useCallback((e: React.WheelEvent<HTMLInputElement>) => {
+ if (!disableScrollChange || type !== "number" || originalProps.isReadOnly || originalProps.isDisabled) {
+ return;
+ }
+ const input = e.currentTarget;
+ input.blur();
+ }, [disableScrollChange, type, originalProps.isReadOnly, originalProps.isDisabled]);📝 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 handleScrollingChange = (e: React.WheelEvent<HTMLInputElement>) => { | |
| if (disableScrollChange) { | |
| (e.target as HTMLInputElement).blur(); | |
| } | |
| }; | |
| const handleScrollingChange = useCallback((e: React.WheelEvent<HTMLInputElement>) => { | |
| if (!disableScrollChange || type !== "number" || originalProps.isReadOnly || originalProps.isDisabled) { | |
| return; | |
| } | |
| const input = e.currentTarget; | |
| input.blur(); | |
| }, [disableScrollChange, type, originalProps.isReadOnly, originalProps.isDisabled]); |
|
|
Ok thanks @wingkwong |
3 participants
Closes #4238
📝 Description
added a prop disableScrollChange to disable change in the input when the type is number
⛳️ Current behavior (updates)
disable change in number on scrolling with mouse wheel or touchpad, since it can change value without user noticing when in scrollable container
🚀 New behavior
prop added to disable if needed
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
disableScrollChangeto enhance input control during scrolling.