feat(core): add window zoom status bar item and fix secondary window zoom#17164
feat(core): add window zoom status bar item and fix secondary window zoom#17164
Conversation
Resolves GH-17163 - Show a prominent status bar item with zoom-in/zoom-out icon when zoom level is not default - Provide interactive tooltip with zoom controls (+/-, reset, open preferences) - Extract window zoom preference keys as reusable constants - Update zoom level preference description with correct localization and step size - Extract Chromium's zoom base factor as a named constant (ZoomLevel.ZOOM_BASE)
Resolves GH-11843 - Route setZoomLevel IPC via windowName to target specific secondary windows - Apply current zoom level to newly created secondary windows
There was a problem hiding this comment.
Thanks for the improvement @ndoschek. Works great! I have a few inline comments and a general remark:
- imho the dialog should stay open if the + and - buttons are pressed, this way i could increase the zoom multiple times, coudl you check this?
- Also is there a reason, why the background of this item is different from the other statusbar items (light theme)? (EDIT: i believe this is fixed with #17161, right?)
| if (e.preferenceName === PREF_WINDOW_ZOOM_LEVEL) { | ||
| const zoomLevel = this.electronWindowPreferences.get(PREF_WINDOW_ZOOM_LEVEL, 0); | ||
| this.updateWindowZoomLevel(zoomLevel); | ||
| } |
There was a problem hiding this comment.
ElectronSecondaryWindowService is bound as a singleton that lives for the entire application lifecycle, so the preference listener doesn't need explicit disposal - it should remain active as long as the application is running.
|
Thanks for the review @sgraband! Ad 1: I don't think keeping the tooltip open on +/- would actually improve the UX here. Since each zoom step changes the page's zoom level, the status bar item itself shifts position, which means the tooltip would also need to reposition. The user would have to move their mouse to the new location anyway, at which point the tooltip reappears with the updated zoom level. So the current behavior of closing and re-showing on hover feels natural for this case IMO. Ad 2: The different background is intentional - the item uses the prominent status bar colors, ( |
- Use lazy tooltip function to avoid leaking React roots on every zoom change - Add missing `name` property for accessibility and status bar context menu - Remove unnecessary empty constructor in WindowZoomActionBar
0ea411b to
e662892
Compare
sgraband
left a comment
There was a problem hiding this comment.
Thanks for the explanation! LGTM 👍
What it does
Resolves GH-17163
Resolves GH-11843
Zoom status bar item:
Secondary window zoom fix:
How to test
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers