fix: label placement when the select has a placeholder or description #4125
Conversation
* fix(use-image): image ReferenceError in SSR * fix(use-image): sync with beta * fix(use-image): sync with beta * chore(use-image): remove unnecessary comments
* fix(docs): routing.mdx * Delete .changeset/pre.json
🦋 Changeset detectedLatest commit: 95728e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
@tianenpang is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces a patch for the Changes
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 and nitpick comments (2)
packages/components/select/src/use-select.ts (1)
317-318: Consider extracting label placement logic for better maintainabilityThe label placement condition could be more maintainable if extracted into a separate function.
Consider refactoring like this:
+const shouldPlaceLabelOutside = ({ + labelPlacement, + hasPlaceholder, + description, + isMultiline, +}: { + labelPlacement: SelectVariantProps["labelPlacement"]; + hasPlaceholder: boolean; + description?: ReactNode; + isMultiline?: boolean; +}) => { + if (labelPlacement === "outside-left") return true; + if (labelPlacement !== "outside") return false; + + return !(hasPlaceholder || !!description) || !!isMultiline; +}; -const shouldLabelBeOutside = - labelPlacement === "outside-left" || - (labelPlacement === "outside" && - (!(hasPlaceholder || !!description) || !!originalProps.isMultiline)); +const shouldLabelBeOutside = shouldPlaceLabelOutside({ + labelPlacement, + hasPlaceholder, + description, + isMultiline: originalProps.isMultiline, +});packages/components/select/__tests__/select.test.tsx (1)
726-731: Test case should cover more scenarios.While the test case correctly verifies label placement with
isMultilineandlabelPlacement="outside", it would be beneficial to add more test cases to cover:
- Label placement with description but without placeholder
- Label placement with placeholder but without description
- Label placement with both description and placeholder
+ it("should place the label outside when labelPlacement is outside with description", () => { + const labelContent = "Favorite Animal Label"; + render( + <Select + isMultiline + aria-label="Favorite Animal" + data-testid="select" + label={labelContent} + labelPlacement="outside" + description="Select your favorite animal" + > + <SelectItem key="penguin">Penguin</SelectItem> + </Select>, + ); + const base = document.querySelector("[data-slot=base]"); + const trigger = document.querySelector("[data-slot=trigger]"); + expect(base).toHaveTextContent(labelContent); + expect(trigger).not.toHaveTextContent(labelContent); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
.changeset/odd-tomatoes-call.md(1 hunks)packages/components/select/__tests__/select.test.tsx(1 hunks)packages/components/select/src/use-select.ts(1 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/odd-tomatoes-call.md
🔇 Additional comments (4)
packages/components/select/src/use-select.ts (2)
317-318: LGTM: Accessibility is properly maintained
The label placement changes preserve accessibility by maintaining proper ARIA attributes and label associations. The logic ensures that the label remains semantically connected to the select element regardless of its visual placement.
317-318: Verify label placement behavior in edge cases
The updated label placement logic now considers both placeholder and description, but we should verify the behavior in these scenarios:
- When both placeholder and description are present
- When isMultiline is true with a placeholder
- When switching between states dynamically
✅ Verification successful
Label placement logic is correctly implemented
The updated label placement logic in use-select.ts is correct and consistent with the component's behavior across the codebase:
-
The condition
labelPlacement === "outside-left" || (labelPlacement === "outside" && (!(hasPlaceholder || !!description) || !!originalProps.isMultiline))properly handles all edge cases:- When
labelPlacementis "outside-left", label is always outside - When
labelPlacementis "outside", label is outside if:- Neither placeholder nor description exists, OR
isMultilineis true
- When
-
This is verified by:
- Test coverage in
select.test.tsxspecifically testing this behavior - Consistent usage across documentation and examples
- Similar implementation pattern in related components (Input, DatePicker, etc.)
- Fix history in CHANGELOG.md addressing label placement behavior
- Test coverage in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering these scenarios
rg -A 5 "describe.*label.*placement" || \
rg -A 5 "test.*label.*outside"
# Search for related component usage
ast-grep --pattern 'Select $props={
$$$
labelPlacement: $_,
placeholder: $_,
description: $_
$$$
}'
Length of output: 183
Script:
#!/bin/bash
# Let's try a broader search for test files related to Select component
fd "select.*.test" --type f
# Search for label placement logic tests with more context
rg -A 10 "labelPlacement.*outside"
# Look for actual component usage examples
rg -A 5 "<Select.*labelPlacement"
# Search for the specific hook usage
ast-grep --pattern 'useSelect({
$$$
labelPlacement: $_,
$$$
})'
Length of output: 145635
packages/components/select/stories/select.stories.tsx (2)
360-396: LGTM! The new section effectively demonstrates label placement with description.
The implementation properly showcases all three label placement variants (default, outside, outside-left) with both placeholder and description props, which directly addresses the PR's objective.
363-372: Verify prop consistency across variants.
All three Select components maintain consistent props (color, description, label, placeholder, variant) while only differing in their labelPlacement prop, which is good for comparison purposes.
Also applies to: 373-383, 384-394
✅ Verification successful
Props are consistently used across label placement variants
After reviewing the code in the stories file, I can confirm that the Select components maintain consistent props (color, description, label, placeholder, variant) across all three label placement variants ("inside", "outside", "outside-left"). This is clearly demonstrated in the LabelPlacementTemplate component where all three variants are shown side by side with identical props, ensuring a proper comparison.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Select component's props are used consistently across the codebase
# Look for any inconsistent usage of description prop with different label placements
rg -U "description=.*labelPlacement" packages/components/select/
Length of output: 66
Script:
#!/bin/bash
# Let's check the story file content to verify the Select components and their props
cat packages/components/select/stories/select.stories.tsx
# Also check for other Select usages with description and labelPlacement
rg -U "labelPlacement.*description|description.*labelPlacement" packages/components/select/
Length of output: 26507
Closes #
📝 Description
Fixed label placement when the
Selecthas aplaceholderordescription.⛳️ Current behavior (updates)
Corrected the rendering position and elements nesting.
🚀 New behavior
Updated the conditions for
shouldLabelBeOutside.💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
Related to PR#3853.
Before
After
Summary by CodeRabbit
Release Notes
New Features
Selectcomponent when using placeholders or descriptions.Selectcomponent.Bug Fixes
Documentation
Selectcomponent with descriptions and placeholders for better user context.