feat(select): adding isClearable to select #4785
Conversation
…ding-isClearble-to-select-recreation
🦋 Changeset detectedLatest commit: 2e8228f 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 |
|
@abhinav700 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces two new properties—isClearable and onClear—to the Select component across several packages. This update enables the component to display a clear button when applicable and to execute a callback when the selection is cleared. The changes span component implementation, its interaction hook, theming adjustments, documentation updates including a new demo, and Storybook story enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SC as Select Component
participant US as useSelect Hook
participant OC as onClear Callback
U->>SC: Click clear button (if isClearable is true)
SC->>US: Trigger getClearButtonProps / handleClear
US->>SC: Clear selected keys (resetting selection)
US->>OC: Invoke onClear callback (if provided)
SC->>U: Update UI to reflect the cleared state
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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: 0
🧹 Nitpick comments (5)
apps/docs/content/components/select/is-clearable.raw.jsx (1)
19-53: Enhance accessibility and type safety of the PetBoldIcon component.Consider the following improvements:
- Add aria-label to the SVG for better accessibility.
- Add PropTypes or convert to TypeScript for better type safety.
Apply this diff to enhance the component:
export const PetBoldIcon = (props) => { return ( <svg aria-hidden="true" + aria-label="Pet icon" fill="none" focusable="false" height="1em" role="presentation" viewBox="0 0 24 24" width="1em" {...props} >packages/components/select/src/select.tsx (1)
73-84: Consider RTL support and design system consistency.The end content implementation has two potential issues:
- The
end-18class seems arbitrary and might not align with the design system.- The positioning might need adjustment for RTL layouts.
Consider using design system spacing tokens and RTL-aware classes:
- <div className="flex end-18"> + <div className="flex end-4 rtl:start-4">packages/core/theme/src/components/select.ts (1)
32-49: Consider using design system tokens for spacing and opacity.The clearButton styling could be improved:
- The
mb-4class seems arbitrary.- The opacity values (0.70) could be standardized using design system tokens.
Consider using design system tokens:
- "mb-4", + "mb-3", "relative", "start-auto", "appearance-none", "outline-none", "select-none", - "opacity-70", + "opacity-[--opacity-60]", - "hover:!opacity-100", + "hover:!opacity-[--opacity-100]", "cursor-pointer", - "active:!opacity-70", + "active:!opacity-[--opacity-60]",.changeset/three-socks-glow.md (1)
5-5: Add missing article "the".For better readability, consider adding "the" before "prop":
-add `isClearable` and `onClear` prop to Select component (#2239 +add `isClearable` and `onClear` props to the Select component (#2239🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: ...addisClearableandonClearprop to Select component (#2239(AI_EN_LECTOR_MISSING_DETERMINER_THE)
apps/docs/content/docs/components/select.mdx (1)
673-676: Add return type to event description.Consider adding the return type to the event description for consistency with other events:
- description: "Handler that is called when the clear button is clicked." + description: "Handler that is called when the clear button is clicked.", + default: "-"🧰 Tools
🪛 LanguageTool
[uncategorized] ~676-~676: Loose punctuation mark.
Context: ... when the clear button is clicked." }, { attribute: "onChange", ...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/three-socks-glow.md(1 hunks)apps/docs/content/components/select/index.ts(2 hunks)apps/docs/content/components/select/is-clearable.raw.jsx(1 hunks)apps/docs/content/components/select/is-clearable.ts(1 hunks)apps/docs/content/docs/components/select.mdx(3 hunks)packages/components/select/src/select.tsx(3 hunks)packages/components/select/src/use-select.ts(8 hunks)packages/components/select/stories/select.stories.tsx(2 hunks)packages/core/theme/src/components/select.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/content/components/select/is-clearable.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/three-socks-glow.md
[uncategorized] ~5-~5: You might be missing the article “the” here.
Context: ...add isClearable and onClear prop to Select component (#2239
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
apps/docs/content/docs/components/select.mdx
[uncategorized] ~676-~676: Loose punctuation mark.
Context: ... when the clear button is clicked." }, { attribute: "onChange", ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (10)
apps/docs/content/components/select/index.ts (1)
34-34: LGTM!The changes correctly integrate the new
isClearablefeature into the select component's documentation structure.Also applies to: 70-70
apps/docs/content/components/select/is-clearable.raw.jsx (1)
55-65: LGTM!The demo component effectively showcases the
isClearablefeature with a good selection of options.packages/components/select/src/select.tsx (1)
65-71: LGTM!The clearButton implementation using useMemo is efficient and correctly handles the conditional rendering based on isClearable and selected items.
packages/core/theme/src/components/select.ts (2)
132-132: LGTM!The size variants for the clearButton are well-defined and consistent across different sizes.
Also applies to: 137-138, 142-143
183-187: LGTM!The isClearable variant correctly handles the visibility and opacity of the clear button based on the filled state.
packages/components/select/src/use-select.ts (3)
138-142: LGTM! Well-documented prop addition.The new
onClearprop is well-documented with a clear description of its purpose and behavior.
349-353: LGTM! Proper implementation of clear functionality.The
handleClearimplementation correctly:
- Clears selection using
setSelectedKeys- Calls the
onClearcallback- Returns focus to the select component
725-739: LGTM! Accessible clear button implementation.The
getClearButtonPropsimplementation follows accessibility best practices:
- Proper ARIA role and label
- Keyboard navigation support
- Focus management
packages/components/select/stories/select.stories.tsx (2)
58-62: LGTM! Proper Storybook configuration.The
isClearableproperty is correctly added to argTypes with appropriate control type.
1392-1398: LGTM! Clear example story.The Clearable story provides a good example of using the new feature with proper props.
|
@wingkwong can you check this new PR please |
| @@ -0,0 +1,65 @@ | |||
| import {Select, SelectItem} from "@nextui-org/react"; | |||
There was a problem hiding this comment.
please update the package name
There was a problem hiding this comment.
are you referring to @nextui-org part?
There was a problem hiding this comment.
yes. now we've rebranded. please change to @heroui/react
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@heroui/select": patch | |||
There was a problem hiding this comment.
@heroui/theme package?
wingkwong
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/components/select/src/use-select.ts (3)
138-142: Improve documentation for onClear prop.The documentation should clarify that this callback is only invoked when
isClearableis true and a value is selected./** * Callback fired when the value is cleared. - * if you pass this prop, the clear button will be shown. + * This callback is only invoked when isClearable is true and a value is selected. + * @see isClearable */
349-358: Add error handling and type safety to handleClear.The implementation could be improved with better error handling and type safety.
const handleClear = useCallback(() => { + if (!domRef.current) { + console.warn('Select ref is not available'); + return; + } state.setSelectedKeys(new Set([])); onClear?.(); - domRef.current?.focus(); + domRef.current.focus(); }, [onClear, state]);
725-739: Enhance clear button accessibility.The clear button's accessibility could be improved by adding aria-hidden and disabled states when no value is selected.
const getClearButtonProps: PropGetter = useCallback( (props = {}) => { + const hasSelectedValue = state.selectedItems?.length > 0; return { ...props, type: "button", tabIndex: -1, + disabled: !hasSelectedValue, "aria-label": "clear selection", + "aria-hidden": !hasSelectedValue, "data-slot": "clear-button", "data-focus-visible": dataAttr(isClearButtonFocusVisible), className: slots.clearButton({class: clsx(classNames?.clearButton, props?.className)}), ...mergeProps(clearPressProps, clearFocusProps), }; }, - [slots, isClearButtonFocusVisible, clearPressProps, clearFocusProps, classNames?.clearButton], + [slots, isClearButtonFocusVisible, clearPressProps, clearFocusProps, classNames?.clearButton, state.selectedItems], );packages/components/select/__tests__/select.test.tsx (2)
949-1065: Add test cases for edge cases.The clear button visibility tests are thorough but should include additional scenarios:
- Clear button behavior when the select is disabled
- Keyboard interaction with the clear button
Add these test cases:
it("should not show clear button when select is disabled", async () => { const wrapper = render( <Select disableAnimation isClearable isDisabled isOpen aria-label="Favorite Animal" data-testid="select" label="Favorite Animal" defaultSelectedKeys={["penguin"]} onClear={jest.fn()} > <SelectItem key="penguin">Penguin</SelectItem> </Select> ); const select = wrapper.getByTestId("select"); const buttons = select.querySelectorAll("button"); expect(buttons.length).toEqual(0); }); it("should support keyboard interaction with clear button", async () => { const onClear = jest.fn(); const wrapper = render( <Select disableAnimation isClearable isOpen aria-label="Favorite Animal" data-testid="select" label="Favorite Animal" defaultSelectedKeys={["penguin"]} onClear={onClear} > <SelectItem key="penguin">Penguin</SelectItem> </Select> ); const select = wrapper.getByTestId("select"); const clearButton = select.querySelector("button"); await user.tab(); expect(clearButton).toHaveFocus(); await user.keyboard("{Enter}"); expect(onClear).toHaveBeenCalledTimes(1); });
1067-1132: Add test cases for multiple selection and form integration.The clear button functionality tests should include:
- Clearing multiple selections
- Integration with form submission
Add these test cases:
it("should clear multiple selections when clear button is clicked", async () => { const onClear = jest.fn(); const wrapper = render( <Select disableAnimation isClearable isOpen aria-label="Favorite Animal" data-testid="select" label="Favorite Animal" selectionMode="multiple" onClear={onClear} > <SelectItem key="penguin">Penguin</SelectItem> <SelectItem key="zebra">Zebra</SelectItem> </Select> ); const select = wrapper.getByTestId("select"); const listboxItems = wrapper.getAllByRole("option"); await user.click(listboxItems[0]); await user.click(listboxItems[1]); expect(select).toHaveTextContent("Penguin, Zebra"); const clearButton = select.querySelector("button"); await user.click(clearButton); expect(select).not.toHaveTextContent("Penguin"); expect(select).not.toHaveTextContent("Zebra"); }); it("should clear form value when clear button is clicked", async () => { const onSubmit = jest.fn((e) => e.preventDefault()); const wrapper = render( <form onSubmit={onSubmit}> <Select disableAnimation isClearable aria-label="Favorite Animal" data-testid="select" label="Favorite Animal" name="animal" defaultSelectedKeys={["penguin"]} > <SelectItem key="penguin">Penguin</SelectItem> </Select> <button type="submit" data-testid="submit">Submit</button> </form> ); const select = wrapper.getByTestId("select"); const submit = wrapper.getByTestId("submit"); await user.click(submit); expect(onSubmit).toHaveBeenCalledWith( expect.objectContaining({ target: expect.any(HTMLFormElement), currentTarget: expect.any(HTMLFormElement), }) ); const clearButton = select.querySelector("button"); await user.click(clearButton); await user.click(submit); expect(onSubmit).toHaveBeenCalledWith( expect.objectContaining({ target: expect.any(HTMLFormElement), currentTarget: expect.any(HTMLFormElement), }) ); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/select/__tests__/select.test.tsx(1 hunks)packages/components/select/src/use-select.ts(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (2)
packages/components/select/src/use-select.ts (2)
21-21: LGTM!The
usePressimport is correctly added to support the clear button interaction.
404-411: LGTM!The dependencies are correctly updated to include
isClearablefor proper reactivity.
this PR is duplicate of #3746
it adds a clear functionality to the select component.
Summary by CodeRabbit