-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(components): prevent label shift when error message appears with … #5800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
fix(components): prevent label shift when error message appears with … #5800
Conversation
…outside placement - Fix label positioning issue in Select, Input, and NumberInput components - Label now maintains stable position when error message appears/disappears - Wrapped trigger/input element in relative positioned container - Error messages are now outside the label's positioning context Fixes heroui-inc#5796
🦋 Changeset detectedLatest commit: e993a6a The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
@hassanzadeh-mj is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds an additional positioned wrapper (style: position: relative) around the existing input/trigger wrapper when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component as Input/NumberInput/Select
participant DOM as Render Tree
User->>Component: Render(props with shouldLabelBeOutside)
alt shouldLabelBeOutside = true
Component->>DOM: Create outer div (position: relative)
Component->>DOM: Render Label (outside / before inner wrapper)
Component->>DOM: Render inner wrapper (input/trigger + contents)
Component->>DOM: Render helper/description/error (outside label's positioning context)
else shouldLabelBeOutside = false
Component->>DOM: Render wrapper (input/trigger + inline Label)
Component->>DOM: Render helper/description/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/components/number-input/src/number-input.tsx (1)
108-113
: LGTM! Code duplication detected.The relative-positioned wrapper correctly implements the label positioning fix. The structure matches the pattern in Input component.
This is the same wrapper pattern duplicated from Input (lines 87-92). See the comment on
input.tsx
for a suggested refactor to extract this common pattern into a reusable utility component.packages/components/select/src/select.tsx (1)
158-172
: LGTM! Same duplication concern as other components.The relative-positioned wrapper correctly isolates the trigger's positioning context from error messages. The label rendering logic appropriately places the label before the trigger when outside, and inside when not.
Like Input and NumberInput, this pattern could benefit from extraction into a shared utility. See the comment on
input.tsx
for a suggested approach.
🧹 Nitpick comments (1)
packages/components/input/src/input.tsx (1)
87-92
: LGTM! Consider extracting the common wrapper pattern.The relative-positioned wrapper correctly isolates the label's positioning context from the error message, achieving the PR objective. The nested structure ensures the label uses the wrapper as its positioning ancestor rather than a container that includes the error message.
However, this exact pattern is duplicated in NumberInput and Select components. Consider extracting a reusable wrapper component or utility function to reduce duplication and improve maintainability.
Example extraction:
// shared utility component const LabelPositioningWrapper = ({ children, shouldWrap }: { children: React.ReactNode; shouldWrap: boolean; }) => { if (shouldWrap) { return <div style={{position: "relative"}}>{children}</div>; } return <>{children}</>; };Then use it consistently across Input, NumberInput, and Select:
if (shouldLabelBeOutside) { return ( <div {...getMainWrapperProps()}> - <div style={{position: "relative"}}> + <LabelPositioningWrapper shouldWrap> <div {...getInputWrapperProps()}> {!isOutsideLeft && !isOutsideTop ? labelContent : null} {innerWrapper} </div> - </div> + </LabelPositioningWrapper> {helperWrapper} </div> ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/input/src/input.tsx
(1 hunks)packages/components/number-input/src/number-input.tsx
(1 hunks)packages/components/select/src/select.tsx
(1 hunks)
🔇 Additional comments (1)
packages/components/select/src/select.tsx (1)
159-161
: Verify label positioning for alllabelPlacement
values.
Confirm thatshouldLabelBeOutside
is true for every outside placement (top, top-start, top-end, outside, outside-left) and that the updated DOM order doesn’t break the visual layout or the label’s absolute‐positioning CSS.
…outside placement
Fixes #5796
Closes #
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit