refactor: use onPress event instead (component-wise) #4354
Conversation
🦋 Changeset detectedLatest commit: 4120d2e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request involve updating the event handling for various components across the codebase, specifically transitioning from the deprecated Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (6)
.changeset/two-ghosts-search.md (1)
1-5: Fix typo and enhance changeset descriptionThe changeset has a typo and could benefit from a more detailed description:
- "decrepate" should be "deprecated"
- Consider adding more details about:
- The reason for the change
- Whether this affects existing implementations
- Migration steps if needed
Apply this diff:
--- "@nextui-org/alert": patch --- -replace decrepate onClick in Alert +replace deprecated onClick with onPress in Alert component + +This change: +- Updates event handling to use onPress instead of onClick +- Maintains backward compatibility +- Aligns with the component library's event handling patternspackages/components/card/__tests__/card.test.tsx (1)
44-52: Consider consolidating duplicate test casesThere are two test cases testing the same functionality:
- "should be clicked when is pressable" (lines 31-40)
- "should trigger onPress function" (lines 44-52)
Consider consolidating these tests to improve maintainability.
- it("should be clicked when is pressable", async () => { - const onPress = jest.fn(); - const {getByRole} = render(<Card disableRipple isPressable onPress={onPress} />); - - const button = getByRole("button"); - - await user.click(button); - - expect(onPress).toHaveBeenCalled(); - }); - - it("should trigger onPress function", async () => { + it("should trigger onPress when clicked and pressable", async () => { const onPress = jest.fn(); const {getByRole} = render(<Card disableRipple isPressable onPress={onPress} />); const button = getByRole("button"); await user.click(button); expect(onPress).toHaveBeenCalled(); });packages/components/button/__tests__/button.test.tsx (1)
Line range hint
1-85: Consider adding touch event testsSince we're moving to onPress events, which are more touch-friendly, consider adding test cases for touch events to ensure proper behavior on touch devices.
Example test case to add:
it("should trigger onPress function on touch", async () => { const onPress = jest.fn(); const {getByRole} = render(<Button disableRipple onPress={onPress} />); const button = getByRole("button"); // Simulate touch events fireEvent.touchStart(button); fireEvent.touchEnd(button); expect(onPress).toHaveBeenCalled(); });packages/components/menu/stories/menu.stories.tsx (1)
48-48: LGTM: Event handler updated correctly with a suggestionThe change from
onClicktoonPressis implemented correctly. However, consider improving the demo implementation.Consider using a more descriptive demonstration instead of the
alertfunction:- <MenuItem key="new" onPress={() => alert("[onPress] New file")}> + <MenuItem + key="new" + onPress={() => console.log("[MenuItem:onPress] New file clicked")} + >This would provide better visibility in the browser's console and follow common development practices for debugging and demonstration purposes.
packages/components/menu/__tests__/menu.test.tsx (1)
Line range hint
320-343: LGTM! Consider adding more test cases.The test case effectively verifies both
onPressandonActionhandlers. Consider adding additional test cases to verify:
- Event handling when the item is disabled
- Event propagation order between
onPressandonActionpackages/components/dropdown/stories/dropdown.stories.tsx (1)
151-151: Consider updating the alert message for consistency.The alert message could be updated to reflect the event type being used.
- <DropdownItem key="new" onPress={() => alert("New file")}> + <DropdownItem key="new" onPress={() => alert("[onPress] New file")}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.changeset/two-ghosts-search.md(1 hunks)packages/components/alert/src/alert.tsx(1 hunks)packages/components/button/__tests__/button-group.test.tsx(1 hunks)packages/components/button/__tests__/button.test.tsx(1 hunks)packages/components/button/stories/button.stories.tsx(1 hunks)packages/components/card/__tests__/card.test.tsx(1 hunks)packages/components/dropdown/stories/dropdown.stories.tsx(1 hunks)packages/components/link/__tests__/link.test.tsx(1 hunks)packages/components/link/stories/link.stories.tsx(1 hunks)packages/components/listbox/stories/listbox.stories.tsx(1 hunks)packages/components/menu/__tests__/menu.test.tsx(1 hunks)packages/components/menu/stories/menu.stories.tsx(1 hunks)
🔇 Additional comments (5)
packages/components/button/__tests__/button-group.test.tsx (1)
29-31: LGTM! Event handler update is correct
The change from onClick to onPress is consistent with the refactoring objective while maintaining the same test coverage and verification logic.
packages/components/alert/src/alert.tsx (1)
76-76: Verify impact of onClick to onPress migration
The change from onClick to onPress on the close button is consistent with the refactoring objective. However, we should verify if this change affects existing consumers of the Alert component.
✅ Verification successful
Change from onClick to onPress is safe
The change from onClick to onPress on the close button is safe as there are no direct consumers using the onClick prop. The only usage of Alert component found in the codebase is in stories, where it uses onClose prop for handling close events, which is already aligned with the new implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of Alert component's onClick handler
rg -l "onClick.*Alert"
# Search for any spread props passed to Alert that might contain onClick
rg "const.*props.*=.*{.*onClick.*}.*\n.*<Alert.*{\.\.\.props}" -U
Length of output: 250
Script:
#!/bin/bash
# Search for Alert component usage patterns
rg -A 2 "<Alert.*onClick"
# Search for potential spread props usage with Alert
rg -A 2 "<Alert.*\.\.\."
# Search for test files that might be affected
rg -t test "Alert.*onClick"
# Look for Alert component stories or examples
fd -e stories.tsx -e stories.ts -x rg -l "Alert.*onClick" {}
Length of output: 1598
packages/components/button/stories/button.stories.tsx (1)
85-85: LGTM: Event handler updated correctly
The change from onClick to onPress is implemented correctly and maintains the component's state management functionality.
packages/components/link/stories/link.stories.tsx (1)
61-61: LGTM: Event handler updated correctly
The change from onClick to onPress is implemented correctly in the PressableTemplate component, maintaining proper state management and event logging.
packages/components/listbox/stories/listbox.stories.tsx (1)
177-177: LGTM! Event handler change is consistent.
The change from onClick to onPress is properly implemented and maintains consistency with the alert message format.
Closes #
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Release Notes
New Features
onPressinstead ofonClick, enhancing user interaction consistency across the application.Bug Fixes
onClickfunctionality in the Alert component to ensure compatibility with the latest library updates.Tests
onClicktoonPressfor various components, ensuring accurate testing of user interactions.