feat(input): add outside-top prop #3660
Conversation
🦋 Changeset detectedLatest commit: c7246b1 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 |
|
@abhinav700 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 WalkthroughThis update enhances 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
Outside diff range, codebase verification and nitpick comments (1)
packages/components/input/src/input.tsx (1)
20-20: Add documentation for theisOutsideTopprop.The
isOutsideTopprop is used to control label positioning in theInputcomponent but lacks documentation. Please add comments or documentation to clarify its purpose and usage.
- File:
packages/components/input/src/input.tsx- File:
packages/components/input/src/use-input.tsAnalysis chain
New prop
isOutsideTopadded.The
isOutsideTopprop is introduced to control the label positioning. Ensure this prop is documented and used consistently.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage and documentation of `isOutsideTop` prop. # Test: Search for documentation or comments related to `isOutsideTop`. rg --type md --type js --type ts --type tsx 'isOutsideTop'Length of output: 86
Script:
#!/bin/bash # Description: Verify usage and documentation of `isOutsideTop` prop. # Test: Search for documentation or comments related to `isOutsideTop`. rg --type markdown --type javascript --type typescript --type tsx 'isOutsideTop'Length of output: 115
Script:
#!/bin/bash # Description: Verify usage and documentation of `isOutsideTop` prop. # Test: Search for documentation or comments related to `isOutsideTop` across all files. rg 'isOutsideTop'Length of output: 444
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- .changeset/little-crabs-destroy.md (1 hunks)
- packages/components/input/src/input.tsx (6 hunks)
- packages/components/input/src/use-input.ts (2 hunks)
- packages/components/input/stories/input.stories.tsx (3 hunks)
- packages/core/theme/src/components/input.ts (1 hunks)
Additional comments not posted (11)
.changeset/little-crabs-destroy.md (1)
1-6: Changeset is well-written and accurate.The changeset correctly describes the addition of the
outside-topprop and its purpose, along with the affected packages.packages/components/input/src/input.tsx (3)
85-87: Conditional rendering logic updated.The logic now considers
isOutsideTopfor label rendering. This enhances flexibility but ensure it doesn't introduce regressions.
52-57: Styling changes for error messages and descriptions.The background color for error messages and descriptions has been updated. Ensure these changes align with the design guidelines.
120-120: Conditional label rendering withisOutsideTop.The label is conditionally rendered based on
isOutsideLeftorisOutsideTop. Ensure all scenarios are covered in tests.packages/components/input/src/use-input.ts (3)
264-264: New variableisOutsideTopintroduced.The
isOutsideTopvariable is used to manage label positioning. Ensure this is consistently applied across the component.
268-279: Refined logic forisLabelOutside.The logic for
isLabelOutsidehas been refined to incorporateoutside-top. Ensure this logic aligns with the intended behavior.
253-256: Enhanced label positioning logic withoutside-top.The logic for determining label placement now includes
outside-top. Ensure this logic is thoroughly tested.Verification successful
Test coverage for
outside-toplabel placement is present.The
outside-toplabel placement is covered in theinput.stories.tsxfile, indicating it is demonstrated and tested through story examples. Ensure these stories are reviewed for completeness in visual testing.
packages/components/input/stories/input.stories.tsx: Demonstratesoutside-toplabel placement.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for enhanced label positioning logic. # Test: Search for test cases covering `outside-top` label placement. rg --type ts --type tsx 'outside-top'Length of output: 65
Script:
#!/bin/bash # Description: Verify test coverage for enhanced label positioning logic. # Test: Search for test cases covering `outside-top` label placement. rg --type=ts --type=tsx 'outside-top'Length of output: 65
Script:
#!/bin/bash # Description: Verify test coverage for enhanced label positioning logic. # Test: Search for test cases covering `outside-top` label placement across all files. rg 'outside-top'Length of output: 900
packages/components/input/stories/input.stories.tsx (3)
52-52: Addition of "outside-top" to labelPlacement options is correct.The inclusion of
"outside-top"in theoptionsarray aligns with the PR objective to enhance label positioning. This change is consistent with existing options.
181-181: Addition of "outside-top" label placement in the story is appropriate.The new
Inputcomponent withlabelPlacement="outside-top"ensures that the new feature is demonstrated and testable in the story.
200-205: Inclusion of "outside-top" with placeholder is well-implemented.Adding this
Inputcomponent tests theoutside-toplabel placement with a placeholder, ensuring comprehensive coverage.packages/core/theme/src/components/input.ts (1)
173-178: "outside-top" layout configuration is well-structured.The addition of
outside-topin thelabelPlacementvariant introduces a new layout that is consistent with the design goals. The CSS classes are appropriately defined to achieve the desired positioning.
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)
- apps/docs/content/components/input/label-placements.ts (1 hunks)
- apps/docs/content/docs/components/input.mdx (1 hunks)
Additional comments not posted (2)
apps/docs/content/components/input/label-placements.ts (1)
8-8: Addition ofoutside-toplabel placement is correct.The new label placement option
"outside-top"has been added correctly to the array. This enhances the flexibility of theInputcomponent.apps/docs/content/docs/components/input.mdx (1)
78-78: Documentation foroutside-toplabel placement is accurate.The documentation correctly includes the new
outside-topoption forlabelPlacement, providing clear guidance on its usage.
wingkwong
changed the title
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/little-crabs-destroy.md (1 hunks)
- packages/components/input/src/input.tsx (3 hunks)
- packages/components/input/src/use-input.ts (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- .changeset/little-crabs-destroy.md
- packages/components/input/src/input.tsx
- packages/components/input/src/use-input.ts
|
Can anyone merge this? @jrgarciadev @wingkwong please? |
This PR is scheduled for v2.5.0. We will review and merge after v2.4.7. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.changeset/little-crabs-destroy.md (1)
6-6: Enhance the changeset description for better changelog clarityWhile the current description is concise, it would be helpful to include more details about the feature's purpose and impact.
Consider expanding the description like this:
-introduce `outside-top` prop to input (#3058) +introduce `outside-top` prop to input allowing label placement outside regardless of placeholder (#3058)packages/components/input/package.json (1)
Line range hint
3-3: Consider semantic versioning for feature additionGiven that this PR introduces a new feature (
outside-topprop), consider whether a minor version bump (2.3.0) would be more appropriate than a patch version (2.2.5), following semantic versioning principles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.changeset/little-crabs-destroy.md(1 hunks)packages/components/input/package.json(2 hunks)
🔇 Additional comments (1)
packages/components/input/package.json (1)
39-39: Verify the minimum theme version requirement
The peer dependency update for @nextui-org/theme to >=2.2.12 appears to be related to the new label positioning feature. Let's verify this is the minimum version required.
✅ Verification successful
Theme dependency update is justified
The theme dependency update to >=2.2.12 is necessary as the codebase shows the new outside-top label positioning feature requires theme support, evidenced by:
- The feature is implemented in the theme component (
packages/core/theme/src/components/input.tsincludesoutside-topstyling) - The input component relies on this theme implementation (
packages/components/input/src/use-input.tsreferencesisOutsideTop)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check when the theme changes for label positioning were introduced
# Search for label-related changes in the theme package
rg -p "label.*outside.*top" packages/components/theme/
Length of output: 123
Script:
#!/bin/bash
# Let's try a broader search for theme-related label positioning
# First, find all theme-related files
fd -e js -e ts -e tsx theme
# Then search for label positioning in theme files
rg -p "label.*position"
# Also check for any recent changes or additions related to label positioning
rg -p "outside.*top"
Length of output: 6814
There was a problem hiding this comment.
seems this is only useful for "without placeholder". for "with placeholder, basically this prop is same as outside. I'm wondering if this would confuse users or not. @ryo-manba what do you think?
|
marking this PR as |
to resolve this, I think we can replace |
|
@wingkwong |
@abhinav700 please pull the latest code and add this note. Then we'll have a final review. Thanks. |
abhinav700
force-pushed
the
fix-3058/adding_outside-top_prop
branch
from
c8a6a0c to
c6f2658
Compare
abhinav700
force-pushed
the
fix-3058/adding_outside-top_prop
branch
from
c6f2658 to
c7246b1
Compare
|
@wingkwong I have pulled the code and updated the docs |
Closes #3058
📝 Description
⛳️ Current behavior (updates)
In old behaviour, the label was presented inside the input component if
placholder === ""even iflabelPlacement === "outside"🚀 New behavior
Adding outside-top prop which dispalys the label outside the input component regardless of whether the placeholder is there or not.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
labelPlacementproperty.