feat(textarea): introduce isClearable#3477
Conversation
🦋 Changeset detectedLatest commit: 05df8ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (5)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 |
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
- please add changeset
- please add test
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.
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
please also include the corresponding info in documentation
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: 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.
|
@IsDyh01 the clear button should be visible only when the textarea has some content
please take a look at the input behavior https://nextui.org/docs/components/input#clear-button |
I already took care of it |
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: Simplify clear button classes.Based on previous review discussions, some of these positioning classes appear to be redundant. Consider simplifying to only include the necessary classes for positioning.
className: slots.clearButton({ class: clsx( classNames?.clearButton, props?.className, - isMultiline ? (isFilled ? "relative block opacity-100 p-0 -m-0 end-0" : "") : "", + isMultiline ? (isFilled ? "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 (2)
packages/components/input/src/use-input.ts (2)
372-372: LGTM! Proper layout adjustments for textarea.
The padding and layout changes correctly handle the positioning requirements for the textarea component.
Also applies to: 415-419
536-544:
Add missing dependency to useCallback.
The useCallback hook is missing the slots and classNames?.headerWrapper dependencies, which could lead to stale closures.
const getHeaderWrapperProps: PropGetter = useCallback(
(props = {}) => {
return {
...props,
className: slots.headerWrapper({class: clsx(classNames?.headerWrapper, props?.className)}),
};
},
- [slots, classNames?.headerWrapper],
+ [slots, classNames?.headerWrapper],
);Likely invalid or redundant comment.
|
mark it on hold until the doc structure revamp pr got merged to beta branch |
| class: clsx( | ||
| classNames?.inputWrapper, | ||
| isFilled ? "is-filled" : "", | ||
| isMultiline ? "flex-col items-start gap-0" : "", |
There was a problem hiding this comment.
@IsDyh01 Tailwind classes should be exclusively defined in the theme package, specifically in the input.ts file. This is because the TailwindCSS content path is configured to target only the theme package
e.g.
// tailwind.config.js
import {nextui} from "@nextui-org/react";
/** @type {import('tailwindcss').Config} */
const config = {
content: [
// ...
// make sure it's pointing to the ROOT node_module
"./node_modules/@nextui-org/theme/dist/**/*.{js,ts,jsx,tsx}" // <- theme package
],
theme: {
extend: {},
},
darkMode: "class",
plugins: [nextui()]
}
export default config;
| ### Clear Button | ||
|
|
||
| If you pass the `isClearable` property to the textarea, it will have a clear button at the | ||
| end of the textarea, it will be visible when the textarea has a value. | ||
|
|
||
| <CodeDemo title="Clear Button" files={textareaContent.clearButton} /> |
There was a problem hiding this comment.
@IsDyh01 Please move this above the description example.
| export * from "./warning"; | ||
| export * from "./danger"; | ||
| export * from "./success"; | ||
| export * from "./trash"; |
|
@IsDyh01 I added some comments |
|
ok, I will modify it as soon as possible. |
4 participants
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.