-
Notifications
You must be signed in to change notification settings - Fork 2.2k
ux: improve responsiveness on small widths for auto-approve button #8152
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?
ux: improve responsiveness on small widths for auto-approve button #8152
Conversation
- + Add Russian i18n string
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.
Thank you for your contribution! I've reviewed the changes and found some issues that need attention. The PR successfully addresses the responsiveness issue on narrow screens, but there are a few important items to address.
) : ( | ||
<CheckCheck className="size-3 flex-shrink-0" /> | ||
)} | ||
<CheckCheck className="min-[400px]:hidden size-3 flex-shrink-0" /> |
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.
Is this intentional? The button always shows the CheckCheck icon on narrow screens regardless of the auto-approve state. This could be confusing since users expect the icon to reflect whether auto-approve is on or off. Consider keeping the state-based icon logic even on narrow screens:
<CheckCheck className="min-[400px]:hidden size-3 flex-shrink-0" /> | |
<span className="min-[400px]:hidden"> | |
{!effectiveAutoApprovalEnabled ? ( | |
<X className="size-3 flex-shrink-0" /> | |
) : ( | |
<CheckCheck className="size-3 flex-shrink-0" /> | |
)} | |
</span> |
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.
That is intentional, it keeps the semantic meaning of the button (double checkmark == auto-approve) even without the text on the button that says what it does
@@ -173,24 +173,35 @@ export const AutoApproveDropdown = ({ disabled = false, triggerClassName = "" }: | |||
"inline-flex items-center gap-1.5 relative whitespace-nowrap px-1.5 py-1 text-xs", | |||
"bg-transparent border border-[rgba(255,255,255,0.08)] rounded-md text-vscode-foreground", | |||
"transition-all duration-150 focus:outline-none focus-visible:ring-1 focus-visible:ring-vscode-focusBorder focus-visible:ring-inset", | |||
"max-[400px]:shrink-0", |
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.
The 400px breakpoint is hardcoded in multiple places (lines 176, 182, 183, 191, 198). Consider extracting this to a constant or CSS variable for better maintainability. For example:
const NARROW_SCREEN_BREAKPOINT = '400px';
// Then use: `max-[${NARROW_SCREEN_BREAKPOINT}]` in template literals
Or define it as a Tailwind config value if the project supports it.
@@ -284,7 +284,8 @@ | |||
"triggerLabel_zero": "0 auto-approve", | |||
"triggerLabel_one": "1 auto-approved", | |||
"triggerLabel_other": "{{count}} auto-approved", | |||
"triggerLabelAll": "YOLO" | |||
"triggerLabelAll": "YOLO", | |||
"triggerLabelOffShort": "Off" |
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.
Good addition! However, this new translation key needs to be added to all other locale files to prevent missing translation warnings. The following locales are missing this key:
- Spanish (es)
- French (fr)
- German (de)
- Chinese (zh-CN)
- Japanese (ja)
- And all other supported locales
Even if you don't know the translations, please add the key with the English value as a placeholder to prevent runtime warnings.
Related GitHub Issue
Follow-up for #7894
Description
I've attached a video demonstrating the change below
Test Procedure
Resize the extension to quite wide and quite narrow
Pre-Submission Checklist
Screenshots / Videos
RvRvpnGui_NdyucCk18T.mp4
Documentation Updates
Get in Touch
I prefer email, so just write all questions as comments for this PR
Important
Improves auto-approve button responsiveness for small screens by adjusting icon and label display in
AutoApproveDropdown.tsx
and updating locale files for compact labels.AutoApproveDropdown.tsx
, added responsive behavior for the auto-approve button to display abbreviated labels on screens narrower than 400px.triggerLabelOffShort
for compact display inen/chat.json
andru/chat.json
.This description was created by
for 5b48385. You can customize this summary. It will automatically update as commits are pushed.