-
-
Notifications
You must be signed in to change notification settings - Fork 7
Fix UI Text Overflow Issues #415
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { cva, type VariantProps } from 'class-variance-authority' | |
| import { cn } from '@/lib/utils' | ||
|
|
||
| const buttonVariants = cva( | ||
| 'inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50', | ||
| 'inline-flex items-center justify-center rounded-md text-sm font-medium ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50', | ||
| { | ||
|
Comment on lines
7
to
9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing Given the PR goal (fix specific overflow cases), consider scoping wrapping behavior to the few problem buttons (or introducing an opt-in/out class/variant) rather than changing the global default. SuggestionConsider keeping the default as const buttonVariants = cva(
'inline-flex items-center justify-center whitespace-nowrap ...',
{
variants: {
wrap: {
true: 'whitespace-normal break-words',
false: 'whitespace-nowrap',
},
// ...
},
defaultVariants: {
wrap: false,
},
}
)Then apply Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this variant and updating the affected call sites. |
||
| variants: { | ||
| variant: { | ||
|
|
||
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.
Using
Tabs.Trigger asChildwith your sharedButtonis a good consolidation, but be careful with styling:Button variant="outline"has its own hover/background/text rules that may conflict with the visual intent of an active tab.Right now you only set
data-[state=active]:bg-primary/80but not the corresponding text color/border color. Depending on theme variables, this can yield low-contrast text or an outline border that looks incorrect when active.Suggestion
Consider fully defining the active state styling (bg + text + border) to avoid theme-dependent contrast issues, e.g.:
If you want the inactive state to remain outlined, keep
variant="outline"and only override the active state.Reply with "@CharlieHelps yes please" if you'd like me to add a commit applying consistent active styles to all four triggers.