feat(select): adding isClearable to select #4785
Conversation
…ding-isClearble-to-select-recreation
🦋 Changeset detectedLatest commit: 57fa473 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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
Suggested reviewers
✨ 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 |
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.
…nextui package to heroui
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/three-socks-glow.md (1)
6-6: Add missing article "the" in the description.For better grammar, consider adding "the" before "prop":
-add `isClearable` and `onClear` prop to Select component (#2239) +add the `isClearable` and `onClear` prop to Select component (#2239)🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: You might be missing the article “the” here.
Context: ...addisClearableandonClearprop to Select component (#2239)(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/three-socks-glow.md(1 hunks)apps/docs/content/components/select/is-clearable.raw.jsx(1 hunks)packages/components/select/__tests__/select.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/content/components/select/is-clearable.raw.jsx
🧰 Additional context used
🪛 LanguageTool
.changeset/three-socks-glow.md
[uncategorized] ~6-~6: You might be missing the article “the” here.
Context: ...add isClearable and onClear prop to Select component (#2239)
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: TypeScript
- GitHub Check: Build
🔇 Additional comments (1)
packages/components/select/__tests__/select.test.tsx (1)
949-1135: Well-structured and comprehensive test coverage for the clear functionality!The test cases thoroughly cover all scenarios for the clear button functionality:
- Visibility conditions (with/without selection, with/without isClearable)
- Callback invocation
- Value clearing behavior
The tests are well-organized, follow testing best practices, and provide good coverage of the new features.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
- in
lgsize, the close button is not aligned with the selector button.
- if using
underlinedvariant, the close button will be relatively close to selector button, which is not consistent with other variants.
- please check all
labelPlacementcases. The button is bit off.
- since you've created a new slot for clearButton, please also update in this section: https://heroui-8rltgh7fz-heroui.vercel.app/docs/components/select#slots
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/docs/content/docs/components/select.mdx (3)
151-157: Clear Button Documentation Addition: Clarity and ConsistencyThe new "Clear Button" section is clearly added and explains that the clear button appears only when a value is selected. For extra clarity, consider mentioning explicitly what happens when no value is selected (i.e. that the clear button remains hidden) and ensure that the accompanying CodeDemo (
selectContent.isClearable) reflects this behavior.
583-588: API Table Update forisClearable: Informative and ConsistentThe API entry for
isClearableis well-documented with a clear type and a default value. You might consider enriching the description by noting that the clear button will only become visible when an item is selected, further reinforcing its behavior as described in the new documentation section.
680-684: API Table Update foronClear: Clear Event Handler DocumentationThe addition of the
onClearevent handler is clear and concise. One minor nitpick: the description ends with a period, which is slightly inconsistent with other table entries. Consider either removing it or ensuring uniform punctuation across all entries for consistency.🧰 Tools
🪛 LanguageTool
[uncategorized] ~684-~684: 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 (1)
apps/docs/content/docs/components/select.mdx(3 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/select.mdx
[uncategorized] ~684-~684: Loose punctuation mark.
Context: ... when the clear button is clicked." }, { attribute: "onChange", ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/content/docs/components/select.mdx (1)
681-685: Add a period at the end of the description.The description for the
onClearevent handler is missing a period at the end, unlike other descriptions in the API documentation.- description: "Handler that is called when the clear button is clicked." + description: "Handler that is called when the clear button is clicked."🧰 Tools
🪛 LanguageTool
[uncategorized] ~685-~685: 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 (3)
apps/docs/content/components/select/is-clearable.raw.jsx(1 hunks)apps/docs/content/docs/components/select.mdx(4 hunks)packages/core/theme/src/components/select.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/content/components/select/is-clearable.raw.jsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/select.mdx
[uncategorized] ~685-~685: Loose punctuation mark.
Context: ... when the clear button is clicked." }, { attribute: "onChange", ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: TypeScript
- GitHub Check: Continuous Release
- GitHub Check: Build
🔇 Additional comments (7)
apps/docs/content/docs/components/select.mdx (3)
151-156: Excellent addition of the clear button feature!The new section clearly explains the functionality of the
isClearableproperty and when the clear button becomes visible to users.
584-589: API documentation properly updated with the new property.The
isClearableproperty is well documented with its type, description, and default value.
326-326: Data attribute documentation properly added.The
clearButtondata attribute is well documented, explaining that it's displayed when theisClearableprop is true.packages/core/theme/src/components/select.ts (4)
33-50: Well-structured styling for the clear button.The clear button slot is thoroughly defined with appropriate styling properties including dimensions, positioning, appearance, and interactive states (hover, active, focus). The use of
dataFocusVisibleClassesensures accessibility support for keyboard navigation.
190-194: Good implementation of the isClearable variant.The
isClearablevariant properly handles the visibility of the clear button based on the component's state, showing it only when there's a value to clear (when the component is filled).
137-147: Size-responsive clear button styling.The clear button styling is appropriately adjusted for different component sizes (sm, md, lg), ensuring visual consistency across all size variants of the select component.
242-242: Animation consistency maintained.Including the clear button in the animation settings ensures that it behaves consistently with the rest of the component during state transitions, maintaining a cohesive user experience.
this PR is duplicate of #3746
it adds a clear functionality to the select component.
Summary by CodeRabbit