fix(input): support both endContent and isClearable at the same time#2305
fix(input): support both endContent and isClearable at the same time#2305kght6123 wants to merge 7 commits into
Conversation
… clearable are specified
🦋 Changeset detectedLatest commit: 6b7fb88 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 ↗︎
|
|
@kght6123 is attempting to deploy a commit to the NextUI Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @kght6123, thank you for submitting this pull request (PR). I will review it shortly |
… clearable are specified
wingkwong
requested review from
wingkwong
and removed request for
jrgarciadev
|
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 WalkthroughThis update modifies the input component to allow simultaneous use of 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
|
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
- please resolve conflicts
- please handle RTL as well. The clear button should be on the left hand side in RTL.
wingkwong
changed the title
|
@kght6123 please resolve conflicts and follow up those comments. thanks. |
|
Hey @kght6123 please update this PR, handle RTL and fix the conflicts 🙏🏻 |
![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 (1)
.changeset/eight-moose-dance.md (1)
6-6: Consider enhancing the changeset description.While the description explains the change, it could be more informative by including:
- The motivation behind the change
- The benefit to users
- Any potential considerations for usage
Here's a suggested enhancement:
-I have made a change so that when both endContent and isClearable are specified, endContent no longer replaces the Clear button. Now, both endContent and the Clear button will be displayed. +Enhanced Input component to support simultaneous display of endContent and Clear button when both endContent and isClearable props are specified. This improvement allows users to maintain custom end content while retaining the clear functionality, providing a more versatile input experience. Previously, the Clear button would replace endContent, limiting component customization options.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.changeset/eight-moose-dance.md(1 hunks)
🔇 Additional comments (1)
.changeset/eight-moose-dance.md (1)
1-4: LGTM! Version bump type is correctly set.
The patch version bump is appropriate as this change enhances functionality without breaking existing behavior.
|
@jrgarciadev @wingkwong I don't have much experience with RTL so I'm learning as I go. I understand that I need to support bidirectional text and that the clear button should appear on the left side when right-to-left (RTL). Currently it's on the right, so I'd like to fix this so that it appears on the left side when RTL. |
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
please try to follow the select one. (ref: pr3746)
|
@wingkwong
Where in PR 3746 should I refer to? Can you give me more details? |
|
@kght6123 that PR does the same thing but in select component. I would prefer the same approach rather maintaining two sets of implementations. |
|
@kght6123 any update? |
|
@wingkwong I saw that you closed this PR with unmerged commits. Is this being implemented somewhere else ? |
|
I think not. I think it was closed bc it was pointing release-next branch which was deleted. One could create a PR with the missing RTL implementation. |
|
Was anything merged with only the isClearable + endContent functionality ? |
|
I think not. |
|
Is there a plan to fix this for any of the next releases ? |
|
depending if someone picks up this PR |
|
omg |
5 participants
📝 Description
Improved UI when both endContent and isClearable are specified.
⛳️ Current behavior (updates)
endContent replaces the Clear button.
🚀 New behavior
Both endContent and Clear buttons are visible.
💣 Is this a breaking change (Yes/No):
No.
Currently, both endContent and isClearable do not work as desired.
It's confusing that endContent is replaced by a Clear button, and I don't think there's any use for it.
📝 Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes