-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add UI font size adjustment with Ctrl+Mouse Wheel #7531
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
Conversation
- Add uiFontSize state management to ExtensionStateContext - Implement Ctrl+Mouse Wheel event handler in App.tsx - Add font size persistence to global settings - Apply font size dynamically using CSS variables - Support font size range from 50% to 200% Fixes #7530
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.
Reviewing my own code because apparently I trust no one, not even myself.
|
|
||
| // Handle Ctrl + Mouse Wheel for font size adjustment | ||
| useEffect(() => { | ||
| const handleWheel = (e: WheelEvent) => { |
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.
Missing test coverage for this wheel event handler. Consider adding tests to verify:
- The font size adjusts correctly with wheel up/down
- The font size is clamped between 50% and 200%
- The preventDefault() is called to prevent browser zoom
| useEffect(() => { | ||
| const handleWheel = (e: WheelEvent) => { | ||
| // Check if Ctrl key is pressed (or Cmd on Mac) | ||
| if (e.ctrlKey || e.metaKey) { |
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 the Cmd+Mouse Wheel behavior on Mac intentional? The PR description and issue #7530 only mention Ctrl+Mouse Wheel, but this also responds to metaKey. If this is intended for Mac support, it should be documented in the PR description.
| } | ||
| }, [tab]) | ||
|
|
||
| // Handle Ctrl + Mouse Wheel for font size adjustment |
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 wheel event handler logic is duplicated. Could we extract this into a custom hook like useUIFontSizeAdjustment to avoid duplication and ensure consistency?
| // Apply the font size as a CSS variable that can be used throughout the app | ||
| document.documentElement.style.setProperty("--ui-font-scale", `${uiFontSize / 100}`) | ||
| // Also apply a direct font-size to the body for immediate effect | ||
| document.documentElement.style.fontSize = `${uiFontSize}%` |
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.
Setting both a CSS variable and direct fontSize might be redundant. Consider using just the CSS variable approach:
| document.documentElement.style.fontSize = `${uiFontSize}%` | |
| if (uiFontSize) { | |
| // Apply the font size as a CSS variable that can be used throughout the app | |
| document.documentElement.style.setProperty("--ui-font-scale", `${uiFontSize / 100}`) | |
| // Use CSS to apply: html { font-size: calc(100% * var(--ui-font-scale)); } | |
| } |
| } | ||
| case "uiFontSize": { | ||
| // Store the UI font size preference | ||
| const fontSize = message.value ?? 100 |
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.
Could we add runtime validation here to ensure fontSize is within the expected range?
| const fontSize = message.value ?? 100 | |
| const fontSize = message.value ?? 100 | |
| // Validate font size is within acceptable range | |
| const validatedFontSize = Math.min(200, Math.max(50, fontSize)) | |
| await updateGlobalState("uiFontSize", validatedFontSize) |
|
Closing this PR because the implementation has a fundamental flaw: it scales the entire UI using document.documentElement.style.fontSize, which affects all UI elements EXCEPT the actual text content that users need to read (code blocks, messages, etc.). The font sizes in the content areas remain unchanged because they have fixed sizes set elsewhere. This defeats the entire purpose of the feature - users need to adjust text readability, not UI element sizes. A proper implementation would need to specifically target text content elements while leaving UI chrome at original sizes. |
Summary
This PR implements UI font size adjustment using Ctrl+Mouse Wheel as requested in #7530.
Changes
uiFontSizestate management to ExtensionStateContextImplementation Details
The implementation allows users to:
Testing
Screenshots/Demo
Users can now adjust the UI font size using Ctrl+Mouse Wheel, making the extension more accessible for users who need larger or smaller text.
Fixes #7530
Important
Add UI font size adjustment with Ctrl+Mouse Wheel, persisting changes across sessions.
App.tsxto adjust font size between 50% and 200% in 5% increments.globalSettingsSchemaandwebviewMessageHandler.uiFontSizetoExtensionStateContextandExtensionStateinExtensionMessage.ts.setUiFontSizefunction inExtensionStateContext.tsxto manage font size state.global-settings.tsand handle updates inwebviewMessageHandler.ts.uiFontSizemessage to persist changes inApp.tsx.This description was created by
for ebbfa6e. You can customize this summary. It will automatically update as commits are pushed.