feat(input): add outside-top prop #3660
Conversation
🦋 Changeset detectedLatest commit: 2d9031c 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. |
WalkthroughThis update enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (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
|
![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. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/components/input/src/input.tsx (1)
116-116: LGTM with a suggestion: Updated condition for outside label rendering.The condition
isOutsideLeft || isOutsideTopcorrectly implements the desired behavior for label placement outside the input. This change aligns with the PR objectives.Consider extracting this condition into a descriptive variable for improved readability:
+ const shouldRenderLabelOutside = isOutsideLeft || isOutsideTop; - {isOutsideLeft || isOutsideTop ? labelContent : null} + {shouldRenderLabelOutside ? labelContent : null}This change would make the code more self-documenting and easier to maintain in the future.
packages/components/input/src/use-input.ts (3)
259-266: LGTM! Consider enhancing the comment for clarity.The addition of
outside-topto theshouldLabelBeOutsidecondition correctly implements the new label placement option. This aligns well with the PR objective.Consider updating the comment to explicitly mention
outside-top:- /* - outside -> label is outside only when some placeholder is there - outside-top, outside-left-> label is outside regardless of placeholder - */ + /* + outside: label is outside only when a placeholder is present + outside-top, outside-left: label is outside regardless of placeholder + */
280-283: LGTM! Consider a minor refactoring for improved readability.The addition of
labelPlacement === "outside-top"to theisLabelOutsidecondition correctly implements the new label placement option, aligning with the PR objective.Consider refactoring the condition for improved readability:
- const isLabelOutside = shouldLabelBeOutside - ? labelPlacement === "outside-left" || - labelPlacement === "outside-top" || - hasPlaceholder || - (labelPlacement === "outside" && hasStartContent) - : false; + const isLabelOutside = shouldLabelBeOutside && ( + ["outside-left", "outside-top"].includes(labelPlacement) || + hasPlaceholder || + (labelPlacement === "outside" && hasStartContent) + );This refactoring reduces repetition and makes the condition more concise.
Missing Type Definitions for "outside-top" Label Placement
The TypeScript type definitions for
labelPlacementdo not include the new"outside-top"option. This omission can lead to type errors and unexpected behavior in components that utilize this prop.
- Files using "outside-top" without TypeScript definitions:
packages/components/input/src/use-input.tsapps/docs/content/components/input/label-placements.tspackages/core/theme/src/components/input.tsPlease update the TypeScript types to include
"outside-top"to ensure type safety and consistency across the codebase.🔗 Analysis chain
Line range hint
1-570: Overall implementation of theoutside-topprop is well-executed.The changes made to implement the
outside-toplabel placement option are consistent, well-integrated, and address the PR objective effectively. The new functionality is implemented without introducing inconsistencies or issues in other parts of the file.To ensure the new
outside-topoption is properly handled throughout the codebase, run the following verification script:This script will help identify any areas in the codebase that might need attention due to the introduction of the new
outside-toplabel placement option.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and handling of the new "outside-top" label placement option. # Test 1: Check for any hardcoded "outside-top" strings that might need updating echo "Checking for hardcoded 'outside-top' strings:" rg --type typescript '"outside-top"' # Test 2: Verify that the new option is properly typed echo "Verifying TypeScript types for 'outside-top':" ast-grep --lang typescript --pattern 'type $_ = $_ | "outside-top"' # Test 3: Check for any switch statements or if-else chains that handle label placement echo "Checking label placement handling in switch statements or if-else chains:" rg --type typescript -A 5 'switch.*labelPlacement' rg --type typescript -A 5 'if.*labelPlacement.*===' # Test 4: Look for any documentation or comments that might need updating echo "Checking for documentation or comments that might need updating:" rg --type typescript -i 'label.*placement'Length of output: 910
Script:
#!/bin/bash # Description: Verify the usage and handling of the new "outside-top" label placement option. # Test 1: Check for any hardcoded "outside-top" strings that might need updating echo "Checking for hardcoded 'outside-top' strings:" rg '"outside-top"' -g "*.ts" -g "*.tsx" # Test 2: Verify that the new option is properly typed echo "Verifying TypeScript types for 'outside-top':" ast-grep --lang typescript --pattern 'type $_ = $_ | "outside-top"' # Test 3: Check for any switch statements or if-else chains that handle label placement echo "Checking label placement handling in switch statements or if-else chains:" rg 'switch.*labelPlacement' -A 5 -g "*.ts" -g "*.tsx" rg 'if.*labelPlacement.*===' -A 5 -g "*.ts" -g "*.tsx" # Test 4: Look for any documentation or comments that might need updating echo "Checking for documentation or comments that might need updating:" rg -i 'label.*placement' -g "*.ts" -g "*.tsx"Length of output: 29050
apps/docs/content/docs/components/input.mdx (2)
78-78: LGTM! Consider adding a brief explanation of the new option.The addition of
outside-topto thelabelPlacementoptions is consistent with the new feature introduced in this PR. Well done on updating the documentation to reflect this change.To enhance clarity for users, consider adding a brief explanation of what
outside-topdoes, similar to how you've described it in the PR description. For example:-You can change the position of the label by setting the `labelPlacement` property to `inside`, `outside`, `outside-left` or `outside-top`. +You can change the position of the label by setting the `labelPlacement` property to `inside`, `outside`, `outside-left` or `outside-top`. The `outside-top` option allows the label to be displayed outside the input component, regardless of whether a placeholder is present.
Line range hint
1-391: Update the API section to include the newoutside-topoptionThe API section of the documentation should be updated to reflect the new
outside-topoption for thelabelPlacementprop.Please update the API section as follows:
| Attribute | Type | Description | Default | | ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------- | - | labelPlacement | `inside` \| `outside` \| `outside-left` | The position of the label. | `inside` | + | labelPlacement | `inside` \| `outside` \| `outside-left` \| `outside-top` | The position of the label. | `inside` |packages/core/theme/src/components/input.ts (1)
172-177: LGTM! Consider adding a transition for smooth label movement.The new "outside-top" variant for labelPlacement is well-implemented and aligns with the feature request. It correctly positions the label outside and on top of the input field.
For consistency with other label placements and to enhance user experience, consider adding a transition to the label movement. This can be achieved by adding the following classes to the label property:
label: "relative text-foreground pb-2", +label: "relative text-foreground pb-2 transition-all duration-200 ease-out motion-reduce:transition-none",This change will ensure smooth animation when the label moves, consistent with other label placements in the component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/docs/content/docs/components/input.mdx (1 hunks)
- packages/components/input/src/input.tsx (3 hunks)
- packages/components/input/src/use-input.ts (2 hunks)
- packages/core/theme/src/components/input.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/components/input/src/input.tsx (3)
20-20: LGTM: New propisOutsideTopadded successfully.The addition of the
isOutsideTopprop aligns with the PR objectives and provides the desired functionality for label placement. The prop name is clear and descriptive.
83-83: LGTM: Updated condition for label rendering.The condition
!isOutsideLeft && !isOutsideTopcorrectly implements the desired behavior for label placement. This change ensures that the label is not rendered inside the input when eitherisOutsideLeftorisOutsideTopis true, aligning with the PR objectives.
Line range hint
1-120: Overall assessment: Changes successfully implement theoutside-topprop.The modifications to the Input component effectively introduce the
outside-topprop and correctly handle label placement as per the PR objectives. The changes resolve the issue described in the linked bug report, allowing the label to be displayed outside the input component without requiring a placeholder.Key points:
- The
isOutsideTopprop is added and integrated into the existing logic.- Label rendering conditions are updated in both the
mainWrapperand the component's return statement.- The implementation is consistent and aligns with the intended functionality.
A minor suggestion was made to improve code readability, but overall, the changes are well-implemented and achieve the desired outcome.
packages/components/input/src/use-input.ts (2)
274-274: LGTM! Consistent implementation of the new label placement option.The introduction of
isOutsideTopis consistent with the existing code structure and necessary for handling the new "outside-top" label placement option.
552-552: LGTM! Consistent exposure of the new label placement state.The addition of
isOutsideTopto the return object of theuseInputhook is consistent with the existing pattern and necessary for consumers to utilize the new label placement option.packages/core/theme/src/components/input.ts (1)
172-177: Summary: New "outside-top" label placement successfully implementedThe new "outside-top" label placement has been successfully added to the input component. This implementation addresses the feature request and provides a new option for positioning labels above the input field.
Key points:
- The new variant is correctly placed within the labelPlacement object.
- The implementation is consistent with the existing code structure.
- Basic functionality for the new placement is in place.
Suggestions for further improvement:
- Add a transition for smooth label movement.
- Consider adding compound variants to handle different input states and configurations.
Overall, this is a solid implementation that enhances the flexibility of the input component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.changeset/little-crabs-destroy.md (1)
1-6: LGTM! Consider enhancing the commit message.The changeset looks good. The minor version bump for both packages is appropriate for introducing a new feature without breaking changes. This aligns well with the PR objectives of adding the
outside-topprop to improve label positioning.Consider making the commit message more descriptive. For example:
feat(input): introduce `outside-top` prop for consistent label placement (#3058) - Allows label to be displayed outside the input component - Improves usability by making label position independent of placeholderThis provides more context about the feature and its benefits.
apps/docs/content/components/input/label-placements.ts (3)
13-23: Improved structure and consistencyThe restructuring of the component's rendering logic enhances readability and maintainability. The explicit rendering of each placement type improves predictability.
Consider extracting the repeated Input components into a separate function or component to further improve code reusability and reduce duplication. For example:
const EmailInput = ({ description, labelPlacement, ...props }) => ( <Input type="email" label="email" description={description} labelPlacement={labelPlacement} {...props} /> );Then use it like:
<EmailInput description="inside" /> <EmailInput description="outside" labelPlacement="outside" />
24-52: Comprehensive demonstration with placeholdersThe addition of inputs with placeholders provides a thorough showcase of the component's capabilities. The consistent structure and placeholder text across all inputs enhance readability and uniformity.
To improve accessibility, consider adding an
aria-labelattribute to each Input component. This can be particularly helpful for screen readers when the visual label is positioned differently. For example:<Input type="email" label="email" description="outside-top" labelPlacement="outside-top" placeholder="Enter your email" aria-label="Email input with label outside and on top" />
Line range hint
1-67: Summary: Successful implementation of new label placement optionThe changes in this file successfully implement the new "outside-top" label placement option for the Input component. The restructured code provides a clear and comprehensive demonstration of all label placement options, both with and without placeholders. This implementation resolves the issue mentioned in the linked bug report and enhances the flexibility of the Input component.
Key improvements:
- Addition of the "outside-top" placement option
- Restructured rendering logic for better readability and maintainability
- Comprehensive examples with and without placeholders
These changes align well with the PR objectives and significantly improve the usability of the Input component.
To further enhance this component, consider:
- Implementing unit tests to ensure the correct rendering of labels in all placement scenarios.
- Adding prop types or TypeScript interfaces to clearly define the expected props for the Input component.
- Creating a storybook story for this component to showcase all possible combinations interactively.
packages/components/input/stories/input.stories.tsx (2)
180-183: LGTM: Comprehensive demonstration of new label placementThe additions to the
LabelPlacementTemplatefunction effectively showcase the new "outside-top" label placement option. The changes maintain consistency with the existing structure and demonstrate the feature both with and without placeholders, which is crucial for understanding its behavior in different scenarios.For improved consistency, consider adding a description prop to the new Input components, similar to the existing ones:
- <Input {...args} labelPlacement="outside-top" /> + <Input {...args} description="outside-top" labelPlacement="outside-top" />This minor change would make the new additions fully consistent with the existing pattern.
Also applies to: 196-209
Line range hint
1-1: Consider adding a dedicated story for "outside-top" placementWhile the current changes effectively demonstrate the new "outside-top" label placement, consider adding a dedicated story or updating the description of the
LabelPlacementstory to highlight this new feature. This would enhance the documentation and make it easier for users to understand and test the new functionality.For example, you could add:
export const OutsideTopLabelPlacement = { render: Template, args: { ...defaultProps, label: "Email", labelPlacement: "outside-top", placeholder: "Enter your email", }, };This addition would provide a quick and focused example of the new feature.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- .changeset/little-crabs-destroy.md (1 hunks)
- apps/docs/content/components/input/label-placements.ts (1 hunks)
- packages/components/input/src/use-input.ts (2 hunks)
- packages/components/input/stories/input.stories.tsx (3 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 (2)
apps/docs/content/components/input/label-placements.ts (1)
8-8: LGTM: New placement option added successfullyThe addition of "outside-top" to the placements array aligns with the PR objective and enhances the component's flexibility.
packages/components/input/stories/input.stories.tsx (1)
52-52: LGTM: New label placement option addedThe addition of "outside-top" to the
labelPlacementoptions aligns well with the PR objectives. This change enhances the configurability of the Input component in Storybook, allowing for better demonstration and testing of the new feature.
…abel size consistent
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
Bug Fixes
Documentation