-
-
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?
Conversation
This commit addresses several UI text overflow issues across the application. - Standardized the mobile breakpoint to 1024px in `components/chat.tsx` to match the CSS media queries. - Removed `whitespace-nowrap` from the base `Button` component in `components/ui/button.tsx` to allow button text to wrap by default. - Refactored the settings tabs in `components/settings/components/settings.tsx` to use the `Button` component, fixing the overflow on smaller screens. - Removed `whitespace-nowrap` from the label in `components/copilot.tsx` to allow long checkbox labels to wrap.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
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.
This PR likely fixes the reported overflow, but it introduces a global behavioral change by removing whitespace-nowrap from the base Button, which may cause regressions across the app. components/chat.tsx could avoid unnecessary re-renders by preventing redundant setIsMobile updates on frequent resize events. The Tabs.Trigger asChild refactor is cleaner, but the active-state styling on the outline buttons is incomplete and may cause contrast/border inconsistencies depending on theme variables.
Summary of changes
Overview
This PR addresses several UI text overflow issues by:
- Updating the “mobile” breakpoint detection in
components/chat.tsxfrom768pxto1024px. - Allowing text wrapping by default:
- Removing
whitespace-nowrapfrom checkbox option labels incomponents/copilot.tsx. - Removing
whitespace-nowrapfrom the baseButtonstyles incomponents/ui/button.tsx.
- Removing
- Refactoring settings tabs in
components/settings/components/settings.tsxto renderTabs.TriggerviaasChildand the shared<Button variant="outline" />component, reducing duplicated class strings and improving consistency.
Key refactors
- Replaced long, duplicated
Tabs.TriggerclassName strings with shared button styles + targeteddata-[state=active]overrides viaclassName="data-[state=active]:bg-primary/80".
| 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', | ||
| { |
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.
Removing whitespace-nowrap from the base Button affects every button in the app, not just the ones that were overflowing. That’s a broad behavior change and may introduce regressions (e.g., buttons in tight toolbars or tables wrapping to two lines and increasing row height).
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.
Suggestion
Consider keeping the default as whitespace-nowrap and adding a variant/utility for wrapping, e.g.:
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 wrap (or className="whitespace-normal") only where needed.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this variant and updating the affected call sites.
| <Tabs.Root value={currentTab} onValueChange={setCurrentTab} className="w-full"> | ||
| <Tabs.List className="grid w-full grid-cols-4 gap-x-2"> | ||
| <Tabs.Trigger value="system-prompt" className="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 bg-primary text-primary-foreground hover:bg-primary/90 h-10 px-4 py-2 data-[state=active]:bg-primary/80">System Prompt</Tabs.Trigger> | ||
| <Tabs.Trigger value="model" className="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 bg-primary text-primary-foreground hover:bg-primary/90 h-10 px-4 py-2 data-[state=active]:bg-primary/80">Model Selection</Tabs.Trigger> | ||
| <Tabs.Trigger value="user-management" className="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 bg-primary text-primary-foreground hover:bg-primary/90 h-10 px-4 py-2 data-[state=active]:bg-primary/80">User Management</Tabs.Trigger> | ||
| <Tabs.Trigger value="map" className="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 bg-primary text-primary-foreground hover:bg-primary/90 h-10 px-4 py-2 data-[state=active]:bg-primary/80">Map</Tabs.Trigger> | ||
| <Tabs.Trigger value="system-prompt" asChild> | ||
| <Button variant="outline" className="data-[state=active]:bg-primary/80">System Prompt</Button> | ||
| </Tabs.Trigger> | ||
| <Tabs.Trigger value="model" asChild> | ||
| <Button variant="outline" className="data-[state=active]:bg-primary/80">Model Selection</Button> | ||
| </Tabs.Trigger> | ||
| <Tabs.Trigger value="user-management" asChild> | ||
| <Button variant="outline" className="data-[state=active]:bg-primary/80">User Management</Button> | ||
| </Tabs.Trigger> | ||
| <Tabs.Trigger value="map" asChild> | ||
| <Button variant="outline" className="data-[state=active]:bg-primary/80">Map</Button> | ||
| </Tabs.Trigger> |
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 asChild with your shared Button is 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/80 but 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.:
<Button
variant="outline"
className="data-[state=active]:bg-primary data-[state=active]:text-primary-foreground data-[state=active]:border-primary"
>
…
</Button>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.
User description
This PR resolves several UI text overflow issues by removing
whitespace-nowrapfrom button and label components, standardizing the mobile breakpoint, and refactoring the settings tabs to use the base Button component.PR created automatically by Jules for task 8696950182480457087 started by @ngoiyaeric
PR Type
Bug fix, Enhancement
Description
Removed
whitespace-nowrapfrom base Button component for text wrappingStandardized mobile breakpoint from 768px to 1024px in Chat component
Refactored settings tabs to use Button component instead of inline styles
Removed
whitespace-nowrapfrom checkbox label in Copilot componentDiagram Walkthrough
File Walkthrough
button.tsx
Remove whitespace-nowrap from base Buttoncomponents/ui/button.tsx
whitespace-nowrapfrom the basebuttonVariantsCVA definitionsingle line
copilot.tsx
Remove nowrap constraint from checkbox labelcomponents/copilot.tsx
whitespace-nowrapfrom checkbox label classNamechat.tsx
Standardize mobile breakpoint to 1024pxcomponents/chat.tsx
checkMobilefunctionsettings.tsx
Refactor settings tabs to use Button componentcomponents/settings/components/settings.tsx
asChildpropdata-[state=active]:bg-primary/80class