Skip to content
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

menu presets for window sizes and zooms #4507

Merged
merged 4 commits into from
Feb 24, 2022

Conversation

dimitropoulos
Copy link
Contributor

@dimitropoulos dimitropoulos commented Feb 17, 2022

Recently in #4384, @develohpanda added some functionality for preset zoom levels. I have found this wildly useful.

Take a PR like #4498.

I had to take all 6 of those screenshots 3 times by the end of the PR. No big deal, but the drag is that I had to take the (nearly!) exact same BEFORE pictures each time because (since, visual comparison) I must have the window size and zoom level match.

That part is what #4384 started, but I added one more for FHD size.

Then, in the next commit, I added the same kind of thing, except for zoom preset levels.

To tie it all together, then in the last commit I added the thing I want most: a single standard preset (only enabled in development mode) that sets the window to FHD and the zoom to 400%:

For reference, that creates a window that looks like this, and is idea for screenshots of the type I need in my daily work (such as the PR mentioned above).
Screenshot_20220217_164947

changelog(Improvements): Adds zoom preset levels and more window size presets

@dimitropoulos dimitropoulos self-assigned this Feb 17, 2022
@dimitropoulos dimitropoulos changed the title menu resets for window sizes and zooms menu presets for window sizes and zooms Feb 17, 2022
try {
zoomFactor = localStorage?.getItem('zoomFactor', 1);
return localStorage?.getItem('zoomFactor', ZOOM_DEFAULT);
Copy link
Contributor

@jackkav jackkav Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createWindow is a main context activity, so most of the time localstorage access doesn't do anything or its inconsistent at best. I know this is legacy, but its good practice to catch this stuff when we see it to lighten the load later. A good rule of thumb is if we are seeing optional chains on mixed context functionality then its probably null half the time. so we could to use the ipc bridge if its important to be stored on either side or a data store that is intended for this context like nedb or a config file, or better yet don't store zoom factor at all. This is my preference since its a dev only tool that breaks contextIsolation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: turns out I was wrong localstorage is a browser API localStorage is a file store that works in main. Safe to ignore my comments.

const actual = Math.min(Math.max(ZOOM_MIN, desired), ZOOM_MAX);

browserWindow.webContents.setZoomLevel(actual);
localStorage?.setItem('zoomFactor', actual);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, the pay off of saving this doesn't seem worth putting it in nedb or pushing it back and forth over the ipc bridge to the renderer

try {
zoomFactor = localStorage?.getItem('zoomFactor', 1);
return localStorage?.getItem('zoomFactor', ZOOM_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: turns out I was wrong localstorage is a browser API localStorage is a file store that works in main. Safe to ignore my comments.

@dimitropoulos dimitropoulos enabled auto-merge (squash) February 24, 2022 01:10
@dimitropoulos dimitropoulos merged commit 4581fc1 into Kong:develop Feb 24, 2022
@dimitropoulos dimitropoulos deleted the feat/resize-to-1080p branch February 24, 2022 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants