feat(tabs): add click handling for tab items in tests and implementation #3917
Conversation
🦋 Changeset detectedLatest commit: fa9c239 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 |
WalkthroughThe pull request introduces a new test case for the 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
|
|
@liaoyinglong is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/components/tabs/src/tab.tsx (1)
81-82: Approve change with type safety improvement suggestionThe modification to accept variable arguments in the
handleClickfunction enhances flexibility and aligns with the PR objectives. It allows for proper forwarding of additional arguments to the click handlers.However, to improve type safety, consider using a more specific type instead of
any[]. For example:const handleClick = (...args: React.MouseEvent<HTMLElement>[]) => { chain(onClick, tabProps.onClick)(...args); // ... rest of the function };This change maintains the flexibility while providing better type checking.
packages/components/tabs/__tests__/tabs.test.tsx (1)
392-413: Good addition of click handling test, with room for improvementThe new test case effectively verifies that click handlers for individual tabs are called correctly. This is a valuable addition to the test suite, aligning well with the PR objective of adding click handling for tab items.
However, consider the following enhancements to make the test more robust:
- Verify tab selection: After clicking a tab, check if it becomes selected (e.g.,
aria-selected="true").- Content visibility: Ensure the content of the selected tab is displayed.
- Naming: Use more descriptive names for click handler mocks, e.g.,
handleItem1Clickinstead ofitem1Click.Example improvement:
it("Tab click should be handled and select the tab", async () => { const handleItem1Click = jest.fn(); const handleItem2Click = jest.fn(); const wrapper = render( <Tabs> <Tab key="item1" data-testid="item1" title="Item 1" onClick={handleItem1Click}> <div data-testid="content1">Content 1</div> </Tab> <Tab key="item2" data-testid="item2" title="Item 2" onClick={handleItem2Click}> <div data-testid="content2">Content 2</div> </Tab> </Tabs>, ); const tab1 = wrapper.getByTestId("item1"); const tab2 = wrapper.getByTestId("item2"); await user.click(tab1); expect(handleItem1Click).toHaveBeenCalled(); expect(tab1).toHaveAttribute("aria-selected", "true"); expect(wrapper.getByTestId("content1")).toBeVisible(); await user.click(tab2); expect(handleItem2Click).toHaveBeenCalled(); expect(tab2).toHaveAttribute("aria-selected", "true"); expect(wrapper.getByTestId("content2")).toBeVisible(); });These changes would provide a more comprehensive test of the tab click functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/components/tabs/tests/tabs.test.tsx (1 hunks)
- packages/components/tabs/src/tab.tsx (1 hunks)
🧰 Additional context used
packages/components/tabs/src/tab.tsx
Outdated
| const handleClick = (...args: any[]) => { | ||
| chain(onClick, tabProps.onClick)(...args); |
There was a problem hiding this comment.
can you share some examples of args?
There was a problem hiding this comment.
No, just ensure that the DOM event can be smoothly passed through.
There was a problem hiding this comment.
This's for tabProps.onClick. There should be no args for handleClick.
There was a problem hiding this comment.
Here, handleClick did not receive event, so tabProps.onClick will not have event either.
There was a problem hiding this comment.
please add a test case to cover this change as well.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I removed the event parameter and updated the tests; please take a look when you have time.
liaoyinglong
force-pushed
the
fix/tab-click
branch
from
9a3550a to
a3ca3f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.changeset/seven-tips-help.md (1)
1-5: Changeset looks good, but consider enhancing the description.The changeset correctly identifies this as a patch update for the "@nextui-org/tabs" package, which aligns with the nature of the changes described in the PR objectives.
Consider expanding the description slightly to provide more context. For example:
- add click handling for tab items in tests and implementation + add click handling for tab items in tests and implementation to improve interactivity and test coverageThis additional detail will be helpful for maintainers and users reviewing the changelog in the future.
packages/components/tabs/__tests__/tabs.test.tsx (1)
392-413: LGTM! Consider enhancing assertions for more robust testing.The new test case "Tab click should be handled" is well-structured and effectively verifies the click handling for tab items. It follows good testing practices by using mock functions and simulating user interactions.
To further improve the test case, consider the following suggestions:
- Check the number of times each click handler is called to ensure they are not triggered multiple times unexpectedly.
- Verify that clicking one tab doesn't trigger the other tab's click handler.
Example of enhanced assertions:
expect(item1Click).toHaveBeenCalledTimes(1); expect(item2Click).not.toHaveBeenCalled(); await user.click(tab2); expect(item2Click).toHaveBeenCalledTimes(1); expect(item1Click).toHaveBeenCalledTimes(1); // Ensure it wasn't called againThese additional checks will make the test more robust and help catch potential issues with event propagation or multiple trigger events.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .changeset/seven-tips-help.md (1 hunks)
- packages/components/tabs/tests/tabs.test.tsx (1 hunks)
- packages/components/tabs/src/tab.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/tabs/src/tab.tsx
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/tabs/__tests__/tabs.test.tsx (1)
392-415: LGTM! Consider enhancing test coverage.The new test case "Tab click should be handled" is well-structured and effectively verifies the basic functionality of tab click handling. It correctly sets up mock functions, renders the component, simulates user interactions, and checks if the click handlers are called with the correct event targets.
To further improve the test coverage, consider the following suggestions:
Add assertions to verify the selected state of tabs after clicks. This ensures that clicking a tab not only triggers the click handler but also updates the component's state correctly.
Test the scenario where a tab is clicked multiple times. This can help verify that repeated clicks don't cause unexpected behavior.
Include a test case for clicking a disabled tab to ensure that the click handler is not called and the tab's state doesn't change.
Here's an example of how you might implement these suggestions:
it("Tab click should be handled", async () => { const item1Click = jest.fn(); const item2Click = jest.fn(); const wrapper = render( <Tabs> <Tab key="item1" data-testid="item1" title="Item 1" onClick={item1Click}> <div>Content 1</div> </Tab> <Tab key="item2" data-testid="item2" title="Item 2" onClick={item2Click}> <div>Content 2</div> </Tab> <Tab key="item3" data-testid="item3" title="Item 3" isDisabled> <div>Content 3</div> </Tab> </Tabs>, ); const tab1 = wrapper.getByTestId("item1"); const tab2 = wrapper.getByTestId("item2"); const tab3 = wrapper.getByTestId("item3"); // Test initial state expect(tab1).toHaveAttribute("aria-selected", "true"); expect(tab2).toHaveAttribute("aria-selected", "false"); // Test clicking tab2 await user.click(tab2); expect(item2Click).toHaveBeenCalledTimes(1); expect(item2Click.mock.lastCall[0].target).toBe(tab2); expect(tab1).toHaveAttribute("aria-selected", "false"); expect(tab2).toHaveAttribute("aria-selected", "true"); // Test clicking tab2 again await user.click(tab2); expect(item2Click).toHaveBeenCalledTimes(2); expect(tab2).toHaveAttribute("aria-selected", "true"); // Test clicking disabled tab await user.click(tab3); expect(tab3).toHaveAttribute("aria-selected", "false"); // Ensure no handler is called for disabled tab });These enhancements will provide a more comprehensive test suite for the tab click handling functionality.
wingkwong
added
🚧 Status: Changes Requested
👀 Status: To Review
and removed
🚧 Status: Changes Requested
labels
liaoyinglong
force-pushed
the
fix/tab-click
branch
from
e894b2f to
fa9c239
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.changeset/seven-tips-help.md (1)
1-5: Consider enhancing the changeset description.
While the current description is accurate, it could be more detailed to better document the changes for future reference. Consider adding specifics about:
- The improvements to the
handleClickfunction for chained handlers - The new test case that verifies tab click handling
Here's a suggested enhancement:
---
"@nextui-org/tabs": patch
---
-add click handling for tab items in tests and implementation
+add click handling for tab items in tests and implementation
+
+- Improved `handleClick` function to properly handle chained click handlers
+- Added test case to verify tab click handling functionalitypackages/components/tabs/__tests__/tabs.test.tsx (1)
392-422: Well-structured test case for tab click handling.
The test case effectively verifies the click handling functionality and state management. The structure is clear with distinct sections for initial state verification, click behavior testing, and multiple click handling.
Consider enhancing the test coverage by:
- Adding assertions for the first tab's click handler (
item1Click) - Verifying the arguments passed to click handlers
- Testing click handler behavior with disabled tabs
Example addition:
// Test clicking tab2 again
await user.click(tab2);
expect(item2Click).toHaveBeenCalledTimes(2);
expect(tab2).toHaveAttribute("aria-selected", "true");
+
+ // Test clicking tab1
+ await user.click(tab1);
+ expect(item1Click).toHaveBeenCalledTimes(1);
+ expect(item2Click).toHaveBeenCalledTimes(2);
+ expect(tab1).toHaveAttribute("aria-selected", "true");
+ expect(tab2).toHaveAttribute("aria-selected", "false");📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .changeset/seven-tips-help.md (1 hunks)
- packages/components/tabs/tests/tabs.test.tsx (1 hunks)
- packages/components/tabs/src/tab.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/tabs/src/tab.tsx
📝 Description
Add click handling for tab items in tests and implementation
⛳️ Current behavior (updates)
Currently, the tab component lacks specific click handling and testing for individual tab items.
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
This change improves the interactivity and testability of the Tabs component by ensuring that clicks on individual tab items are properly handled and can be tested.
References:
Summary by CodeRabbit
New Features
Tabscomponent, ensuring click events trigger the correct handlers.Bug Fixes
Tabcomponent by allowing variable arguments to be passed to click handlers.