feat(textarea): introduce isClearable #3477
Conversation
🦋 Changeset detectedLatest commit: c2b16d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 ↗︎
|
|
@IsDyh01 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe update introduces a clear button to the Changes
Assessment against linked issues
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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/input/src/textarea.tsx (3 hunks)
- packages/components/input/stories/textarea.stories.tsx (1 hunks)
Additional comments not posted (5)
packages/components/input/src/textarea.tsx (4)
6-6: Import statement forCloseFilledIconis correct.The import statement is necessary for the clear button functionality.
137-142: Conditional rendering of the inner wrapper with the clear button usinguseMemois correct.The logic to conditionally render the inner wrapper with the clear button using
useMemois correct.
92-93: Usage ofisClearableandgetClearButtonPropsis correct.The logic to get clear button properties from
useInputand conditionally render the clear button is correct.Verify that
useInputsupportsisClearableandgetClearButtonProps.Verification successful
useInputsupportsisClearableandgetClearButtonProps.The
useInputfunction includes theisClearableproperty and thegetClearButtonPropsmethod, confirming their correct usage in theTextareacomponent.
packages/components/input/src/use-input.ts
const isClearable = !!onClear || originalProps.isClearable;const getClearButtonProps: PropGetter = useCallback(...)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `useInput` supports `isClearable` and `getClearButtonProps`. # Test: Search for the definitions of `useInput`. Expect: Definitions include `isClearable` and `getClearButtonProps`. rg --type typescript 'useInput' -A 10Length of output: 72
Script:
#!/bin/bash # Description: Verify that `useInput` supports `isClearable` and `getClearButtonProps`. # Test: Search for the definitions of `useInput`. Expect: Definitions include `isClearable` and `getClearButtonProps`. rg 'useInput' -g '*.ts*' -A 10Length of output: 7997
Script:
#!/bin/bash # Description: Verify that `useInput` supports `isClearable` and `getClearButtonProps`. # Test: Search for `isClearable` and `getClearButtonProps` in the `useInput` implementation. rg 'isClearable|getClearButtonProps' packages/components/input/src/use-input.tsLength of output: 280
128-135: Conditional rendering of the clear button usinguseMemois correct.The logic to conditionally render the clear button using
useMemois correct.Verify that
getClearButtonPropsis used correctly.Verification successful
The
getClearButtonPropsfunction is used correctly.The function definition ensures that the clear button is accessible and properly styled, providing necessary properties such as
role,tabIndex,aria-label, and others. The usage in the provided code snippet is appropriate.
getClearButtonPropsis defined correctly inpackages/components/input/src/use-input.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `getClearButtonProps` is used correctly. # Test: Search for the usage of `getClearButtonProps`. Expect: Correct usage. rg --type typescript 'getClearButtonProps'Length of output: 77
Script:
#!/bin/bash # Description: Verify that `getClearButtonProps` is used correctly. # Test: Search for the usage of `getClearButtonProps`. Expect: Correct usage. rg --type ts 'getClearButtonProps'Length of output: 1781
Script:
#!/bin/bash # Description: Extract the definition and usage of `getClearButtonProps` in `packages/components/input/src/use-input.ts`. # Extract the definition and usage of `getClearButtonProps`. rg --type ts -A 10 'const getClearButtonProps' packages/components/input/src/use-input.tsLength of output: 540
packages/components/input/stories/textarea.stories.tsx (1)
306-316: Definition of theClearableentity is correct.The function correctly defines the new exported entity with the necessary properties. The
onClearcallback is correctly defined to log a message when the clear action is triggered.
|
@IsDyh01 kindly run |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- .changeset/five-adults-protect.md (1 hunks)
- packages/components/input/tests/textarea.test.tsx (1 hunks)
- packages/components/input/stories/textarea.stories.tsx (1 hunks)
Additional comments not posted (3)
.changeset/five-adults-protect.md (1)
1-5: Changeset file correctly indicates a major version update.The changeset file is well-formed and clearly describes the addition of a new feature to the textarea component, which justifies a major version bump.
packages/components/input/__tests__/textarea.test.tsx (1)
1-34: Test implementation for the clear button is comprehensive and correct.The test checks both the functionality of the clear button and the triggering of the
onClearcallback. It uses appropriate tools and methods for simulating user interactions and verifying the component's state.packages/components/input/stories/textarea.stories.tsx (1)
306-317: Clearable story is well-implemented but consider noting the console usage.The story for the clearable textarea is well-implemented and demonstrates the functionality clearly. However, the use of
console.login theonClearcallback is typical for examples but should be accompanied by a comment noting that this is not intended for production use.
IsDyh01
force-pushed
the
feat/textarea-add-clearButton
branch
from
75dcc18
to
6f92cdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- .changeset/five-adults-protect.md (1 hunks)
- packages/components/input/tests/textarea.test.tsx (1 hunks)
- packages/components/input/src/textarea.tsx (3 hunks)
- packages/components/input/stories/textarea.stories.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- .changeset/five-adults-protect.md
- packages/components/input/tests/textarea.test.tsx
- packages/components/input/src/textarea.tsx
- packages/components/input/stories/textarea.stories.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/components/input/src/textarea.tsx (2)
125-127: LGTM with a minor suggestion: clearButtonContent implementationThe implementation of
clearButtonContentusinguseMemois correct and efficiently handles the conditional rendering of the clear button. However, there's a minor improvement to be made:Add
endContentto the dependency array of theuseMemohook to ensure it updates correctly whenendContentchanges:- }, [isClearable, getClearButtonProps]); + }, [isClearable, getClearButtonProps, endContent]);This change will prevent potential stale renders if
endContentis updated.
147-154: LGTM with a minor suggestion: Rendering logic for clear buttonThe conditional rendering for the clear button is well-implemented and correctly handles different cases. However, there's a small improvement that can be made:
Remove the empty label when
isClearableis true andshouldLabelBeInsideis false:- {shouldLabelBeInside ? labelContent : <label {...getLabelProps()}>{}</label>} + {shouldLabelBeInside ? labelContent : null}This change will eliminate an unnecessary empty label element when it's not needed.
packages/core/theme/src/components/input.ts (1)
72-72: LGTM! Consider updating documentation.The addition of the
headerWrapperslot is a good enhancement that allows for more flexible layouts in the input component. This change aligns well with the PR objectives to introduce a clear button functionality.To ensure proper usage of this new slot, consider updating the component's documentation to include:
- The purpose of the
headerWrapperslot- Example usage demonstrating how to utilize this new slot in various scenarios
This will help developers understand and correctly implement the new functionality in their projects.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/components/input/src/textarea.tsx (5 hunks)
- packages/components/input/src/use-input.ts (4 hunks)
- packages/core/theme/src/components/input.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/input/src/use-input.ts
🧰 Additional context used
🔇 Additional comments (4)
packages/components/input/src/textarea.tsx (4)
6-6: LGTM: TrashIcon import addedThe import of TrashIcon is correctly placed and aligns with the new clear button functionality.
18-18: LGTM: OmittedInputProps updatedThe addition of "isClearButtonFocusVisible" to OmittedInputProps is consistent with the new clear button functionality and prevents this prop from being passed to the HTML element.
88-90: LGTM: New props added for clear button functionalityThe addition of
isClearable,getClearButtonProps, andgetHeaderWrapperPropsto the destructured props is consistent with the implementation of the clear button functionality.
Line range hint
1-171: Overall assessment: Well-implemented clear button functionalityThe changes successfully introduce the clear button functionality to the Textarea component. The implementation is consistent with the existing codebase and follows good practices. A few minor suggestions have been made to further improve the code:
- Add
endContentto the dependency array of theuseMemohook forclearButtonContent.- Remove the empty label when
isClearableis true andshouldLabelBeInsideis false.These small adjustments will enhance the overall quality and efficiency of the component.
…xtui into feat/textarea-add-clearButton
|
I'm very sorry for the late revision. I had something to do in the last two weeks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/components/input/src/use-input.ts (1)
523-529: Consider simplifying the clear button styles.Based on previous discussions, some of these classes might be redundant. Consider testing which ones are actually needed for proper positioning.
Try removing redundant classes:
-isMultiline ? "relative block opacity-100 p-0 -m-0 end-0" : "" +isMultiline ? "relative end-0" : ""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/docs/content/docs/components/textarea.mdx(3 hunks)packages/components/input/package.json(2 hunks)packages/components/input/src/use-input.ts(4 hunks)packages/core/theme/src/components/input.ts(1 hunks)
🔇 Additional comments (7)
packages/components/input/package.json (1)
39-39: LGTM! Verify theme package version.
The peer dependency version bump is appropriate for the new textarea clear button feature.
Let's verify that version 2.1.1 of the theme package exists:
✅ Verification successful
Version bump to >=2.1.1 is appropriate and safe
The theme package version in the workspace is 2.2.11, which is higher than the required version 2.1.1. This version bump is safe and aligns with the package's current state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the required theme package version exists
# Expected: Should find version 2.1.1 or higher in the workspace
# Check the theme package version in the monorepo
cat packages/components/theme/package.json | jq -r .version
Length of output: 132
Script:
#!/bin/bash
# Let's try different approaches to locate the theme package and its version
# First, find any package.json files that might contain the theme package
fd package.json --type f --exec grep -l "@nextui-org/theme" {}
# Then, let's check if the theme package exists in the workspace
fd "theme" --type d
# Also check for any lerna or workspace configuration that might list the packages
find . -maxdepth 1 -type f -name "package.json" -o -name "lerna.json" -o -name "pnpm-workspace.yaml" | xargs cat
Length of output: 8092
Script:
#!/bin/bash
# Let's check the version in the core theme package
cat packages/core/theme/package.json | jq -r .version
# Also check the dependencies of other components to see what version they're using
fd package.json --type f --exec grep -A 1 -B 1 '"@nextui-org/theme"' {}
Length of output: 9178
packages/components/input/src/use-input.ts (2)
373-373: LGTM! Proper use of logical properties for RTL support.
The use of pe-0 (padding-end) follows the modern approach for handling RTL layouts.
415-419: LGTM! Proper layout handling for multiline mode.
The flex column layout with items-start ensures proper alignment of elements in multiline mode.
apps/docs/content/docs/components/textarea.mdx (3)
92-97: LGTM! Clear Button section is well documented.
The new section clearly explains the clear button functionality and its visibility conditions. The inclusion of a code demo helps users understand the implementation.
110-110: LGTM! headerWrapper slot is properly documented.
The slot description accurately explains its purpose of wrapping the label and clear button components.
174-174: LGTM! isClearable prop is well documented.
The property is correctly documented with its type, description, and default value, maintaining consistency with the existing API documentation style.
packages/core/theme/src/components/input.ts (1)
72-72: LGTM! Remember to update the documentation.
The headerWrapper slot implementation looks good. The flex layout with proper spacing and alignment will effectively handle the label and clear button positioning. As mentioned in the previous review, please ensure this new slot is documented in the Textarea component's documentation under the Slots section.
Let's verify the documentation update:
✅ Verification successful
Documentation is properly updated with the new headerWrapper slot
The headerWrapper slot is already documented in the Textarea component's documentation at apps/docs/content/docs/components/textarea.mdx. The documentation clearly lists it under the Slots section with the description:
headerWrapper: Wraps the
labeland theclearButton.
This matches exactly with the implementation in the theme configuration file. No further documentation updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the headerWrapper slot is documented
rg -A 5 "headerWrapper" "docs/"
Length of output: 75
Script:
#!/bin/bash
# Let's first find where the documentation might be located
fd -t f "README|DOCUMENTATION|docs|guide" -i
# Also check for markdown files that might contain component documentation
fd -e md
Length of output: 9878
Script:
#!/bin/bash
# Let's check the textarea documentation in the docs app and components
rg -A 5 "textarea" "apps/docs/"
rg -A 5 "textarea" "packages/components/input/README.md"
# Also check if there's a dedicated textarea component documentation
fd "textarea" packages/components/
Length of output: 33998
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/input/src/use-input.ts (1)
523-529: Consider simplifying clear button styles.The clear button styles for multiline inputs contain potentially redundant classes:
blockis unnecessary withrelative-m-0can be removed as it's equivalent tom-0-isMultiline ? "relative block opacity-100 p-0 -m-0 end-0" : "", +isMultiline ? "relative opacity-100 p-0 m-0 end-0" : "",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/components/input/src/use-input.ts(4 hunks)
🔇 Additional comments (4)
packages/components/input/src/use-input.ts (4)
373-373: LGTM: Padding adjustment for multiline inputs.
The pe-0 class removes padding from the end edge, which helps prevent scrollbar overlap issues in multiline inputs.
415-419: LGTM: Layout adjustments for multiline inputs.
The flex column layout with gap-0 ensures proper vertical stacking of elements in multiline mode.
576-576: LGTM: Exposing header wrapper props.
Properly exports the new getHeaderWrapperProps method for external use.
536-544:
Add missing dependencies to useCallback.
The useCallback hook is missing dependencies that could lead to stale closures.
- }, [slots, classNames?.headerWrapper]);
+ }, [slots, classNames?.headerWrapper, props?.className]);Likely invalid or redundant comment.
|
@wingkwong I've merged the latest code. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.changeset/five-adults-protect.md(1 hunks)packages/components/input/package.json(2 hunks)packages/components/input/src/textarea.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/input/package.json
🔇 Additional comments (4)
.changeset/five-adults-protect.md (1)
7-7: LGTM!
The description follows the conventional commit format and properly references the related issues.
packages/components/input/src/textarea.tsx (3)
6-6: LGTM: Import and type changes are appropriate
The changes correctly set up the clear button functionality by importing the requested X icon and updating the type definition.
Also applies to: 18-18
88-90: LGTM: Clear button implementation is well-structured
The clear button implementation is properly memoized and follows the component's patterns by using the hook's props and methods.
Also applies to: 125-127
147-154: 🛠️ Refactor suggestion
Remove unnecessary empty label element
When isClearable is true and shouldLabelBeInside is false, an empty label is rendered. This should be removed as it adds unnecessary DOM elements.
Apply this change:
- {shouldLabelBeInside ? labelContent : <label {...getLabelProps()}>{}</label>}
+ {shouldLabelBeInside ? labelContent : null}Likely invalid or redundant comment.
| --- | ||
| "@nextui-org/input": patch | ||
| "@nextui-org/shared-icons": patch | ||
| "@nextui-org/theme": patch | ||
| --- |
There was a problem hiding this comment.
Update version type for @nextui-org/input
Since adding isClearable is a new feature rather than a bug fix, the version bump for @nextui-org/input should be minor instead of patch according to semantic versioning.
Apply this diff:
---
-"@nextui-org/input": patch
+"@nextui-org/input": minor
"@nextui-org/shared-icons": patch
"@nextui-org/theme": patch
---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --- | |
| "@nextui-org/input": patch | |
| "@nextui-org/shared-icons": patch | |
| "@nextui-org/theme": patch | |
| --- | |
| --- | |
| "@nextui-org/input": minor | |
| "@nextui-org/shared-icons": patch | |
| "@nextui-org/theme": patch | |
| --- |
Closes #2348
Closes #2112
📝 Description
Currently, the input component has a clear button function, while the textarea component does not
⛳️ Current behavior (updates)
The textarea component does not have a clear button
🚀 New behavior
Add Clear button functionality for the textarea component。If there is no content in the textarea, the clear button will not be displayed. If there is content, the clear button will be displayed after the content. Click the clear button to clear the text in the textarea and call the callback function
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Textareacomponent, allowing users to easily clear the text using a clear button.Textareadocumentation with a new section detailing the clear button functionality.trashicon to the shared icons library.Bug Fixes
Textarea Propsdocumentation to include the newisClearableproperty.Chores
@nextui-org/themepackage to require a newer version.