fix(theme): support description with a single space using zero-width … #4215
Conversation
|
|
@iqingting 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 modifications across various story files for components like Autocomplete, Checkbox, Date Input, and others. The primary focus is on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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
|
iqingting
force-pushed
the
zero-width-space
branch
from
917fbb1 to
704ba52
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/components/date-input/src/date-input-group.tsx (1)
67-69: LGTM! Consider documenting this behaviorThe implementation elegantly handles single-space descriptions using a zero-width space character, which maintains layout consistency without visible content. This aligns well with the PR objectives.
Consider adding a code comment explaining the purpose of the zero-width space character () for future maintainers.
packages/components/select/src/select.tsx (1)
74-76: LGTM! Consider extracting to a shared utilityThe implementation correctly uses the zero-width space approach, maintaining consistency with the date-input component.
Consider extracting this pattern into a shared utility function since it's used across multiple components:
// shared-utils.ts export const renderDescription = (description: ReactNode) => description === " " ? <span>​</span> : description;This would ensure consistent behavior and make future updates easier.
packages/components/checkbox/stories/checkbox-group.stories.tsx (1)
235-242: LGTM! Consider using a more semantic class name.The side-by-side comparison effectively demonstrates the layout behavior with and without descriptions. The implementation aligns well with the PR's objective of supporting single-space descriptions for layout consistency.
Consider renaming the class from
w-full max-w-3xlto something more semantic likestory-containerto better convey its purpose. You can define this in your stories' CSS:- <div className="w-full max-w-3xl flex justify-center gap-4"> + <div className="story-container flex justify-center gap-4">packages/components/radio/stories/radio.stories.tsx (1)
Line range hint
282-285: Remove redundant description prop from args.The description is now passed directly in the render function, making this prop in args redundant.
args: { ...defaultProps, - description: "Please select an option", },packages/components/select/stories/select.stories.tsx (1)
868-890: LGTM! Clean implementation of description variants.The implementation effectively demonstrates different description scenarios, including the new single-space description support. The layout is well-structured and consistent with other stories.
Consider extracting the common props into a variable to improve readability:
render: (args: SelectProps) => { + const commonProps = { + ...args, + label: "Favorite Animal", + placeholder: "Select an animal", + }; return ( <div className="w-full max-w-3xl flex justify-center gap-4"> <Template - {...args} + {...commonProps} description="Select your favorite animal" - placeholder="Select an animal" /> <Template - {...args} + {...commonProps} description="Select your favorite animal" - label="Favorite Animal" - placeholder="Select an animal" /> <Template - {...args} + {...commonProps} description=" " - label="Favorite Animal" - placeholder="Select an animal" /> </div> ); },packages/components/autocomplete/stories/autocomplete.stories.tsx (1)
962-969: LGTM! Consider aligning with select.stories.tsx implementation.The implementation effectively demonstrates the description variants, including the single-space support. However, for consistency across components, consider matching the three variants shown in select.stories.tsx.
Consider aligning with select.stories.tsx by adding a third variant:
render: (props: AutocompleteProps) => { + const commonProps = { + ...props, + label: "Favorite Animal", + placeholder: "Select an animal", + }; return ( <div className="w-full max-w-3xl flex justify-center gap-4"> - <Template {...props} description="Select your favorite animal" /> - <Template {...props} description=" " /> + <Template {...commonProps} description="Select your favorite animal" /> + <Template {...commonProps} description="Select your favorite animal" label="Favorite Animal" /> + <Template {...commonProps} description=" " label="Favorite Animal" /> </div> ); },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
packages/components/autocomplete/stories/autocomplete.stories.tsx(1 hunks)packages/components/checkbox/stories/checkbox-group.stories.tsx(1 hunks)packages/components/date-input/src/date-input-group.tsx(1 hunks)packages/components/date-input/stories/date-input.stories.tsx(1 hunks)packages/components/date-input/stories/time-input.stories.tsx(1 hunks)packages/components/date-picker/stories/date-picker.stories.tsx(1 hunks)packages/components/date-picker/stories/date-range-picker.stories.tsx(1 hunks)packages/components/input-otp/stories/input-otp.stories.tsx(2 hunks)packages/components/input/src/input.tsx(1 hunks)packages/components/input/stories/input.stories.tsx(2 hunks)packages/components/input/stories/textarea.stories.tsx(1 hunks)packages/components/radio/stories/radio.stories.tsx(1 hunks)packages/components/select/src/select.tsx(1 hunks)packages/components/select/stories/select.stories.tsx(1 hunks)
🔇 Additional comments (10)
packages/components/input-otp/stories/input-otp.stories.tsx (2)
8-8: LGTM! Good type safety enhancement.
Adding the InputOtpProps type import improves type safety and code maintainability.
216-223: LGTM! Consistent implementation.
The side-by-side comparison follows the same pattern established in other components, maintaining consistency across the codebase.
packages/components/date-input/stories/time-input.stories.tsx (1)
188-195: LGTM! Well-structured implementation.
The changes maintain consistency with other components while properly demonstrating the layout behavior with descriptions. The use of TimeInputProps type ensures type safety.
packages/components/input/stories/textarea.stories.tsx (1)
217-230: LGTM! Well-structured story implementation.
The WithDescription story effectively demonstrates both cases:
- Component with descriptive text
- Component with single space description
packages/components/date-input/stories/date-input.stories.tsx (1)
213-220: LGTM! Clean and consistent implementation.
The story implementation follows the established pattern and maintains type safety with DateInputProps.
packages/components/radio/stories/radio.stories.tsx (1)
274-281: LGTM! Clean implementation of the WithDescription story.
The story implementation follows the established pattern and effectively demonstrates both description cases.
packages/components/date-picker/stories/date-picker.stories.tsx (1)
492-499: LGTM! Well-structured implementation for description variants.
The implementation effectively demonstrates both regular and single-space descriptions side by side, with proper layout constraints and spacing.
packages/components/input/stories/input.stories.tsx (2)
94-101: LGTM! Comprehensive template for description variants.
The template effectively demonstrates all three variants (with description, with placeholder, and with single space), using proper layout constraints.
601-601: LGTM! Proper integration of the new template.
The WithDescription export correctly uses the new template.
packages/components/date-picker/stories/date-range-picker.stories.tsx (1)
573-580: LGTM! Consistent implementation with other components.
The implementation follows the established pattern, effectively demonstrating both regular and single-space descriptions with proper layout constraints.
| @@ -64,7 +64,9 @@ export const DateInputGroup = forwardRef<"div", DateInputGroupProps>((props, ref | |||
| {isInvalid && errorMessage ? ( | |||
| <div {...errorMessageProps}>{errorMessage}</div> | |||
| ) : description ? ( | |||
| <div {...descriptionProps}>{description}</div> | |||
| <div {...descriptionProps}> | |||
| {description === " " ? <span>​</span> : description} | |||
There was a problem hiding this comment.
- It's not immediately clear for users that
" "causes a span to appear where normally you'd expect text - Doesn't seem ideal for accessibility
There was a problem hiding this comment.
Also this won't work if neighboring inputs have multiple lines of description.
It's not just a style issue; the problem is that when no description is provided, the error messages disrupt the layout. For example, if the form is in a modal, the current behavior causes the modal's height to change as the form's validation status updates. And what I do is set the description to an empty value to reserve space so that the error messages no longer disrupt the layout. https://stackblitz.com/edit/vitejs-vite-mj4jjg?file=src%2FApp.tsx CleanShot.2024-12-05.at.11.00.44.mp4 |
That is definitely not great UI-wise, but I think solving it with some CSS is preferable to the implementation proposed in this PR, for the reasons I mentioned in my previous comments. |
I agree it’s not the best solution, but
|
Sorry for the confusion. I meant that the form height changing is not great. I do think it would be nice if we could come up with a generalized fix, but I don't have any decent ideas. The actual CSS implementation will of course depend on your requirements. If you are interested in discussing a specific CSS solution for your situation, you could try asking in Discord. |
📝 Description
Support
description=" "to maintain a consistent layout when some form controls have descriptions and others do not.⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No): No
📝 Additional Information
Summary by CodeRabbit
Release Notes
New Features
Input,Textarea, andDatePicker, enhancing their presentation with descriptions.InputOtpcomponent, improving prop validation.Improvements
WithDescriptionexports across multiple components for better visual representation.Bug Fixes
RadioGroupcomponent to remove unnecessary properties.