feat(select): add isClearable #3746
Conversation
🦋 Changeset detectedLatest commit: 3c07d24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a new module for a selection component that allows users to choose their favorite animals, featuring a clearable option. It exports a list of animal objects, a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
wingkwong
changed the title
|
@wingkwong for the tests that are failing, I don't think they are related to the changes that I have made. When I tried to run the tests on the canary branch( which is in sync and I have pulled those changes locally), the same tests are failing there also even though I have not made any changes to it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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/select/src/select.tsx (2)
57-65: Approve implementation with a suggestion for clarity.The clear button implementation looks good overall. The use of
useMemofor performance optimization and the conditional rendering logic are correct. However, there's a potential confusion in the usage ofendContent.Consider separating the clear button content from
endContentto avoid confusion. You could introduce a new prop likeclearButtonContentfor customizing the clear button. This would make the API more explicit:const clearButton = useMemo(() => { if (isClearable && state.selectedItems?.length) { return ( <span {...getClearButtonProps()}> {clearButtonContent || <CloseFilledIcon />} </span> ); } return null; }, [isClearable, getClearButtonProps, state, clearButtonContent]);This change would allow users to customize both the clear button and the end content independently.
142-148: Approve changes with a suggestion for comment improvement.The modifications to the component's return statement are correct. The conditional rendering of
endContentand the placement of theclearButtonare appropriate.The comment explaining the
endContentdisplay is helpful, but it could be more concise and grammatically correct. Consider revising it as follows:{/** * Display endContent separately only when not showing the clear button. * Otherwise, use it as the clear button. */}This revision maintains the explanation while improving readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/select/src/select.tsx (4 hunks)
🔇 Additional comments (3)
packages/components/select/src/select.tsx (3)
3-3: LGTM: Import for clear button icon added.The import for
CloseFilledIconis correctly added, which will be used for the new clear button functionality.
33-33: LGTM: New props for clear button functionality added.The
isClearableandgetClearButtonPropsprops have been correctly added to the Select component. These are essential for implementing the new clear button functionality as requested in issue #2239.Also applies to: 35-35
Line range hint
1-156: Verify implementation against PR objectives.The implementation successfully meets the objectives stated in the PR summary and issue #2239:
- The
isClearableoption has been added to the Select component.- Users can now easily clear their selection with a clear button.
- The implementation is similar to the Autocomplete component's functionality.
- This solution is more efficient than using
endContentwith a button.The changes are focused on this single feature enhancement, adhering to the guidelines for submitting a pull request.
To ensure the implementation works as expected across the codebase, please run the following verification script:
This script will help identify any places where the new
isClearableprop is being used and check for any remaining TODO comments related to clear functionality in Select components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/components/select/src/use-select.ts (3)
81-83: Clarify the documentation for theendContentpropConsider rephrasing the comment for better clarity.
Apply the following diff to improve the comment:
* Element to be rendered in the right side of the select. - * if you pass this prop and the `onClear` prop, the passed element - * will have the clear button props and it will be rendered instead of the - * default clear button. + * If `onClear` is provided and an element is passed to this prop, + * the element will receive the clear button props and replace the default clear button.
140-141: Improve documentation consistencyCapitalize the first word in the sentence for consistency.
Apply the following diff:
* Callback fired when the value is cleared. - * if you pass this prop, the clear button will be shown. + * If you pass this prop, the clear button will be shown.
313-316: Simplify boolean conversionThe use of
!!may be unnecessary sinceoriginalProps.isDisabledis already a boolean or undefined.Apply the following diff:
const {pressProps: clearPressProps} = usePress({ - isDisabled: !!originalProps?.isDisabled, + isDisabled: originalProps?.isDisabled, onPress: handleClear, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/docs/content/docs/components/select.mdx (3 hunks)
- packages/components/select/src/use-select.ts (10 hunks)
- packages/core/theme/src/components/select.ts (4 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/select.mdx
[grammar] ~153-~153: There appears to be a superfluous article here.
Context: ... button which will be visible only when the some value is selected. A default clear butt...(THE_SOME)
🔇 Additional comments (13)
apps/docs/content/docs/components/select.mdx (2)
393-393: LGTM: NewisClearableproperty documentedThe
isClearableproperty has been correctly added to the Select Props table. The description is clear and concise, and the default value is accurately specified.This addition ensures that the API documentation is up-to-date with the new feature.
414-414: LGTM: NewonClearevent documentedThe
onClearevent has been correctly added to the Select Events table. The description clearly explains when the event is triggered.This addition completes the documentation for the new Clear Button feature, providing developers with the necessary information to implement and handle the clear functionality.
packages/core/theme/src/components/select.ts (4)
31-47: Addition ofclearButtonslot enhances functionalityThe new
clearButtonslot is well-defined, and the classes applied are consistent with existing styles. This improves theselectcomponent by providing users with the ability to clear selections easily.
122-132:clearButtonsize variants are appropriately setThe
clearButtonsizes forsm,md, andlgvariants are defined, matching the expected sizing conventions of the component.
170-175:isClearablevariant implements clearable functionality correctlyThe
isClearablevariant correctly adjusts theinputandclearButtonstyles based on the state of the input, enabling users to clear their selections when the input is filled.
222-222: Consistent animation handling forclearButtonIncluding the
transition-opacityandmotion-reduce:transition-noneclasses for theclearButtonensures consistent animation behavior with other interactive elements.packages/components/select/src/use-select.ts (7)
20-20: LGTM!The added imports are necessary for the new functionality.
308-311: LGTM!The
handleClearfunction correctly handles clearing the selection and invokingonClear.
323-323: LGTM!Using
useFocusRingto manage focus visibility for the clear button is appropriate.
341-341: LGTM!The
isClearableflag correctly determines if the select is clearable.
360-372: LGTM!Dependencies are correctly listed for the
useMemohook.
700-700: LGTM!Returning
isClearablein the hook's return object.
719-719: LGTM!Exposing
getClearButtonPropsmethod in the hook's return object.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
apps/docs/content/components/select/isClearable.ts (2)
17-49: LGTM: Well-implemented icon component with a minor suggestionThe
PetBoldIconcomponent is well-structured, reusable, and follows React best practices. It includes important accessibility attributes and usescurrentColorfor easy styling.Consider adding a
displayNameto the component for easier debugging:PetBoldIcon.displayName = 'PetBoldIcon';
51-72: LGTM: Well-implemented Select component with isClearable functionalityThe App component effectively demonstrates the use of the
isClearableprop on the Select component. The implementation is clean, follows React best practices, and correctly uses theanimalsdata andPetBoldIcon.Consider adding an
onClearhandler to demonstrate how to respond to clear events:const handleClear = () => { console.log('Selection cleared'); // Add any additional logic here }; <Select // ... other props isClearable={true} onClear={handleClear} > {/* ... SelectItems */} </Select>This addition would provide a more comprehensive example of the
isClearablefunctionality.packages/components/select/stories/select.stories.tsx (1)
1009-1017: Consider the placement of the new "Clearable" story within the file.The new "Clearable" story is currently placed at the end of the file. While this is a valid location, you might want to consider placing it closer to related stories (e.g., near other stories that demonstrate core functionalities of the Select component) for better discoverability and logical grouping.
Consider moving the "Clearable" story to a more appropriate location within the file, such as after the "Default" or "Multiple" stories, to improve the overall organization of the stories.
packages/components/select/src/select.tsx (1)
66-77: Ensure consistent styling for 'endContent' with and without 'clearButton'When
clearButtonis present,endContentis wrapped in a<span>with classms-3inside a<div>with classesflex end-18. WhenclearButtonis not present,endContentis wrapped in a<span>with classmb-4. This could lead to inconsistent styling or alignment ofendContentdepending on whether theclearButtonis shown. Consider making the styling consistent in both cases.packages/core/theme/src/components/select.ts (1)
123-123: Consider adjustingclearButtontext size for large variantThe
clearButtonusestext-largefor bothmdandlgsizes. To maintain proportional scaling, consider usingtext-xlfor thelgsize variant to better reflect the larger size.Apply this diff to adjust the
clearButtontext size for thelgvariant:def select = tv({ variants: { size: { sm: { // ... clearButton: "text-medium", }, md: { // ... clearButton: "text-large", }, lg: { // ... - clearButton: "text-large", + clearButton: "text-xl", }, }, // ... }, // ... });Also applies to: 128-128, 133-133
packages/components/select/src/use-select.ts (2)
81-83: Clarify theendContentprop documentationTo improve readability, consider rephrasing the documentation for clarity.
Apply this diff to rephrase the comment:
/** * Element to be rendered in the right side of the select. - * if you pass this prop and the `onClear` prop, the passed element - * will have the clear button props and it will be rendered instead of the - * default clear button. + * If both `endContent` and `onClear` props are provided, the `endContent` + * element will receive the clear button props and will be rendered + * instead of the default clear button. */
139-143: Capitalize the first word in the documentation commentTo maintain consistency, capitalize the first word in the documentation comment.
Apply this diff to fix the capitalization:
/** * Callback fired when the value is cleared. - * if you pass this prop, the clear button will be shown. + * If you pass this prop, the clear button will be shown. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/docs/content/components/select/isClearable.ts (1 hunks)
- apps/docs/content/docs/components/select.mdx (3 hunks)
- packages/components/select/src/select.tsx (4 hunks)
- packages/components/select/src/use-select.ts (10 hunks)
- packages/components/select/stories/select.stories.tsx (1 hunks)
- packages/core/theme/src/components/select.ts (4 hunks)
🧰 Additional context used
🪛 GitHub Check: ESLint
packages/components/select/src/select.tsx
[warning] 32-32:
'isOutsideLeft' is assigned a value but never used
🔇 Additional comments (22)
apps/docs/content/components/select/isClearable.ts (2)
1-15: LGTM: Well-structured data for the Select componentThe
animalsarray is well-defined with unique keys and descriptive labels, making it suitable for use with the Select component. The export statement is correct, allowing easy import in other parts of the application.
74-82: LGTM: Correct export structureThe export statement is well-structured, allowing for easy import of all components and data in other parts of the application. The use of the spread operator to include all properties from the
reactobject is correct and follows modern JavaScript practices.packages/components/select/stories/select.stories.tsx (2)
1009-1017: LGTM! New "Clearable" story added successfully.The new "Clearable" story effectively demonstrates the
isClearablefunctionality, which aligns with the PR objectives. A few observations:
- The
isClearableprop is correctly set totrue.- The
endContentprop is used to add aPetBoldIcon, which is a nice touch for visual representation.- The story uses the
Templaterender function, maintaining consistency with other stories.This addition will be helpful for developers to understand and test the new clearable feature.
Line range hint
1-1017: Overall, the changes effectively implement the new clearable functionality.The addition of the "Clearable" story successfully demonstrates the new
isClearableprop for the Select component. This implementation aligns well with the PR objectives and provides a valuable example for developers using the component.Key points:
- The new story correctly showcases the
isClearableprop.- The use of
endContentwithPetBoldIconprovides a visual cue for the clearable functionality.- The implementation is consistent with other stories in the file.
Suggestions for further improvement have been made, including demonstrating the
onClearevent handler and considering the story's placement within the file. These enhancements would provide a more comprehensive example of the new functionality and improve the overall organization of the stories.apps/docs/content/docs/components/select.mdx (3)
151-155: New "Clear Button" section added successfullyThe new section effectively documents the
isClearableproperty, explaining its functionality and when the clear button becomes visible. The inclusion of a code demo (isClearable) is helpful for users to understand the implementation.
392-392: API documentation updated withisClearablepropertyThe
isClearableproperty has been correctly added to the Select Props table. The type, description, and default value are accurately documented, maintaining consistency with the existing table format.
413-413: API documentation updated withonCleareventThe
onClearevent has been properly added to the Select Events table. The type and description are accurately documented, providing clear information about when the event handler is called. This addition maintains consistency with the existing table format.packages/components/select/src/select.tsx (3)
3-3: Importing CloseFilledIcon for clear button functionalityThe addition of
CloseFilledIconto the imports is appropriate for implementing the clearable feature.
33-33: Addition of 'isClearable' propThe
isClearableprop is correctly added and will enable the clear button functionality.
58-64: Implementation of clear button logicThe
clearButtonis correctly implemented usinguseMemo, and the conditions ensure it appears only whenisClearableis true and there are selected items.packages/core/theme/src/components/select.ts (3)
32-48:clearButtonslot implementation looks goodThe addition of the
clearButtonslot with appropriate utility classes ensures consistent styling and functionality across the component.
174-178:isClearablevariant added correctlyThe introduction of the
isClearablevariant effectively manages the visibility and opacity of theclearButtonbased on the component's state, enhancing the user experience.
226-226: Appropriate animation settings forclearButtonAdding
transition-opacityandmotion-reduce:transition-noneto theclearButtonunder thedisableAnimation: falsevariant ensures smooth transitions while respecting user preferences for reduced motion.packages/components/select/src/use-select.ts (9)
20-20: Import ofusePressis appropriateThe import of
usePressfrom@react-aria/interactionsis necessary for the clear button functionality.
198-198: Destructuring ofonClearis appropriateIncluding
onClearin the destructured props is necessary for the clear button functionality.
308-317: Implementation ofhandleClearfunction is correctThe
handleClearfunction appropriately clears the selection and invokes theonClearcallback.
323-324: Use ofuseFocusRingfor clear button focus management is appropriateThe focus management for the clear button is correctly implemented using
useFocusRing.
341-341: Assignment ofisClearableprop is correctThe
isClearableprop is properly assigned fromoriginalProps.
360-360: IncludingisClearableinuseMemodependencies is appropriateIncluding
isClearablein theuseMemodependencies ensures that styles are recalculated when this prop changes.Also applies to: 365-372
667-682: Implementation ofgetClearButtonPropsfollows accessibility guidelinesThe
getClearButtonPropsfunction correctly sets accessibility attributes and manages focus for the clear button.
700-700: IncludingisClearablein the returned object is appropriateAdding
isClearableto the returned object fromuseSelectmakes it available for consumer components.
719-719: IncludinggetClearButtonPropsin the returned object is appropriateExposing
getClearButtonPropsallows consumer components to access clear button properties.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/content/components/select/is-clearable.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apps/docs/content/components/select/is-clearable.ts (2)
17-49: Skip comment: Previous review feedback is still applicable.
75-83: LGTM!The export configuration is appropriate for a documentation example.
There was a problem hiding this comment.
clear button shouldn't be focused. you may try the following steps.
- open a select, and choose a value.
- click somewhere else to blur, then press tab. select is focused, close button is not focused (which is expected)
- click the value by clicking clear button
- open a select, and choose a value
- click somewhere else to blur, then press tab. select is focused, close button is focused (which is not expected)
Given that tabIndex is set to -1, that shouldn't happen. WIll look into it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/components/select/package.json (1)
Line range hint
2-2: Version should be bumped to 2.3.0Since this PR adds a new feature (
isClearable), following semver conventions, the version should be bumped from2.2.7to2.3.0.Apply this diff:
- "version": "2.2.7", + "version": "2.3.0",packages/components/select/src/use-select.ts (1)
664-678: Consider enhancing clear button accessibility.While the implementation is solid, consider these accessibility improvements:
- Make the aria-label localizable
- Add aria-hidden when the button is disabled
const getClearButtonProps: PropGetter = useCallback( (props = {}) => { return { ...props, role: "button", tabIndex: -1, - "aria-label": "clear selection", + "aria-label": props["aria-label"] || "clear selection", + "aria-hidden": dataAttr(originalProps?.isDisabled), "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], );packages/components/select/stories/select.stories.tsx (1)
1009-1019: LGTM! Consider removing unrelated props.The implementation of the Clearable story looks good. However, the
endContentprop is not related to the clearable functionality and might confuse users about what makes the select clearable.Consider this simplified version:
export const Clearable = { render: Template, args: { ...defaultProps, isClearable: true, - endContent: <PetBoldIcon />, onClear: () => console.log("Select cleared"), }, };apps/docs/content/docs/components/select.mdx (2)
151-155: Enhance the Clear Button section documentationThe section would benefit from additional details about:
- Mentioning that the clear button appears when either
isClearableis true oronClearis provided- Describing the visual appearance and interaction behavior of the clear button
Consider expanding the documentation:
### Clear Button -If you pass the `isClearable` property to the select, it will have a clear button which will be visible only when a value is selected. +If you pass the `isClearable` property to the select or provide an `onClear` handler, it will display a clear button (✕) next to the selected value. The clear button becomes visible only when a value is selected and allows users to reset their selection with a single click.
413-413: Enhance the onClear event documentationThe event handler documentation could be more detailed to better explain its behavior and relationship with the
isClearableprop.Consider expanding the documentation:
-| onClear | `() => void` | Handler that is called when the clear button is clicked. +| onClear | `() => void` | Handler that is called when the clear button is clicked. This handler can be used independently or in conjunction with the `isClearable` prop to implement custom clear behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/docs/content/docs/components/select.mdx (3 hunks)
- packages/components/select/package.json (1 hunks)
- packages/components/select/src/use-select.ts (9 hunks)
- packages/components/select/stories/select.stories.tsx (1 hunks)
🔇 Additional comments (8)
packages/components/select/package.json (1)
38-38: LGTM! Theme dependency update is appropriate.The update to require
@nextui-org/theme>= 2.3.0 aligns with the new clearable feature requirements.packages/components/select/src/use-select.ts (5)
136-140: LGTM! Well-documented prop addition.The
onClearcallback prop is properly typed and documented with clear JSDoc comments.
305-314: LGTM! Clear button handlers are well-implemented.The implementation follows React Aria's patterns with proper memoization and disabled state handling.
320-321: LGTM! Focus handling aligns with review feedback.The clear button's focus behavior has been implemented as suggested in the past review comments, using
tabIndex=-1while maintaining visual feedback through focus ring.
357-369: LGTM! Proper slots integration.The
isClearablestate is correctly integrated into the slots system with proper dependency tracking.
697-697: LGTM! Return value properly updated.The new properties are correctly exposed in the hook's return value.
Also applies to: 716-716
packages/components/select/stories/select.stories.tsx (1)
1016-1017: LGTM! ESLint disable comment is properly placed.apps/docs/content/docs/components/select.mdx (1)
392-392: LGTM! The isClearable prop is well documentedThe API documentation for the
isClearableprop follows the established format and provides clear information about its purpose and default value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/components/select/src/select.tsx (1)
57-63: Enhance accessibility and styling of the clear buttonWhile the implementation is good, consider these improvements:
- Add an aria-label to make the button's purpose clear to screen readers
- Wrap the icon in a span for better styling control
Apply this diff:
- return <button {...getClearButtonProps()}>{<CloseFilledIcon />}</button>; + return ( + <button {...getClearButtonProps()} aria-label="Clear selection"> + <span className="select-clear-button"> + <CloseFilledIcon /> + </span> + </button> + );packages/components/select/src/use-select.ts (1)
136-140: Update the JSDoc comment foronClearprop.The current documentation is inconsistent with the implementation. The clear button visibility depends on the
isClearableprop, not the presence ofonClear.Update the JSDoc comment to:
/** - * Callback fired when the value is cleared. - * if you pass this prop, the clear button will be shown. + * Callback fired when the selection is cleared. + * This callback is invoked when the clear button is clicked. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/components/select/src/select.tsx (4 hunks)
- packages/components/select/src/use-select.ts (9 hunks)
🔇 Additional comments (4)
packages/components/select/src/select.tsx (2)
3-3: LGTM: Import and prop additions are well-structuredThe new imports and props are correctly implemented to support the clearable functionality.
Also applies to: 32-32, 35-35
153-153: Verify RTL support for the clear button placementThe integration looks good, but let's ensure proper RTL support for the clear button placement.
packages/components/select/src/use-select.ts (2)
665-679: LGTM: Accessibility implementation for clear button.The implementation follows accessibility best practices:
- Non-focusable clear button (tabIndex: -1)
- Proper ARIA labeling
- Visual feedback through focus ring
305-309: Review focus management in clear action.The automatic focus on the select after clearing might not be the best UX for all scenarios, especially for keyboard users who might want to continue navigating without the select being focused.
Consider making the focus behavior configurable. Let's verify if other similar components have this behavior:
@wingkwong I have fixed this issue |
| clearButton: [ | ||
| "w-4", | ||
| "h-4", | ||
| "z-10", | ||
| "mb-4", | ||
| "relative", | ||
| "start-auto", |
There was a problem hiding this comment.
Could you align the input's clear button?
It looks like there's no hover style applied.
| className="max-w-xs my-5" | ||
| isClearable={true} | ||
| label="Favorite Animal" | ||
| endContent={<PetBoldIcon />} |
There was a problem hiding this comment.
I think it’s clearer not to specify endContent, and it’s more common as an example.
There was a problem hiding this comment.
@ryo-manba Should I make the same changes to story as well? Or leave it as it is for development purposes?
| isClearable={true} | ||
| label="Favorite Animal" | ||
| endContent={<PetBoldIcon />} | ||
| onClear={() => console.log("Select cleared")} |
There was a problem hiding this comment.
Should I remove the entire line or just leave the onClear as an empty function?
Closes #2239
📝 Description
Added clear button functionality to select component.
Summary by CodeRabbit
New Features
PetBoldIconcomponent to enhance the dropdown menu.isClearableproperty andonClearevent handler for the Select component.Bug Fixes