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

Use default electron implementation for zoomin, zoomout and resetzoom #240

Merged
merged 1 commit into from
Oct 19, 2016

Conversation

razzeee
Copy link
Contributor

@razzeee razzeee commented Aug 11, 2016

This is a possible followup to #187

The three zoom related menu items are now handled by electron itself.

@razzeee razzeee mentioned this pull request Aug 11, 2016
3 tasks
@jasonblais
Copy link
Contributor

Great, thank you @razzeee!!

Would we have two people in the community to help test the pull request? You can download the builds below:

Other artifacts can also be found here.

@yuya-oc
Copy link
Contributor

yuya-oc commented Aug 19, 2016

On OS X, the shortcuts are working. But this PR requires Electron v1.3. Because of discussion in #214, we should not use v1.3 now. So unfortunately we should not merge this.

@jasonblais
Copy link
Contributor

Once this is merged, will it resolve some of the zoom in/out issues on the desktop app? Or would that be a separate issue?

image
Above: When increasing font size, the chat area somehow shrinks to the top left corner. When decreasing font size, it expands towards bottom right causing the scrollbar to be hidden

^Tested on 1.3 and it reproduced there too, so not a new issue. Doesn't always repro, maybe 30% of the time for me on Windows 10

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 8, 2016

@razzeee Now we can consider this PR. Possibly does this resolve the issue reported on the previous comment and #334?

@razzeee
Copy link
Contributor Author

razzeee commented Oct 8, 2016

It might, we'll need to test it

@razzeee
Copy link
Contributor Author

razzeee commented Oct 12, 2016

Rebased, but doesn't seem to solve #334

@jasonblais
Copy link
Contributor

jasonblais commented Oct 13, 2016

Tested on Windows, can confirm that it doesn't solve #334.

For the PR itself, only the following shortcuts work:

  • zoom in: CTRL + =
  • zoom out: CTRL + MINUS
  • reset zoom: CTRL+0

Specifically, CTRL+SHIFT+= and CTRL+PLUS no longer work for zooming in.

EDIT: Used these artifacts to test: https://circleci.com/gh/Razzeee/desktop/53#artifacts

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 13, 2016

Probably need to add some hidden accelerators for zooming in.

@jasonblais
Copy link
Contributor

If we add two accelerators for CTRL+SHIFT+= and CTRL+PLUS, propose we also add CTRL+SHIFT+MINUS for zooming out at the same time

@razzeee
Copy link
Contributor Author

razzeee commented Oct 13, 2016

This PR wasn't about adding featues/keystrokes. It should be exactly the same we already have, see https://github.com/electron/electron/blob/9c19b4e3d5bd9a17d46b09e7ee1ebd6fc2fefc30/lib/browser/api/menu-item-roles.js#L119

@jasonblais
Copy link
Contributor

CTRL+SHIFT+= and CTRL+PLUS already work though, so we shouldn't remove that functionality.

I'm fine adding CTRL+SHIFT+MINUS on a separate PR in the future.

@razzeee
Copy link
Contributor Author

razzeee commented Oct 13, 2016

  • CTRL+SHIFT+= never did work for me, probably due to german keyboard
  • CTRL+PLUS does still works for me

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 17, 2016

Tested on Windows 10 with japanese keyboard.

  • CTRL+SHIFT+= is not applicable because the combination is impossible on the keyboard.
  • CTRL+PLUS works as CTRL+SHIFT+;. CTRL+; also works.

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 17, 2016

On english keyboard, CTRL+PLUS works as CTRL+SHIFT+=, and CTRL+= also works.
I noticed one thing, the behavior is different depends on focus inside of the window.

zoom

When clicking a tab bar, the behavior is same with original one. But when clicking the webview, only it is zoomed, and the region is correct.

@yuya-oc yuya-oc added All Platforms null and removed Pending labels Oct 17, 2016
@jasonblais
Copy link
Contributor

I started with a completely fresh install, and it seems to be working now..

Thanks again @razzeee and @yuya-oc, ready to merge.

One interesting note: I tried with win64.zip and #334 wasn't reproducing. It only reproduced on the installer.

@jasonblais jasonblais added this to the v3.5.0 milestone Oct 18, 2016
@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 19, 2016

@jasonblais Please let me confirm, did you met the behavior like my previous gif?

@jasonblais
Copy link
Contributor

When clicking a tab bar, the behavior is same with original one. But when clicking the webview, only it is zoomed, and the region is correct.

Sorry for missing the note below the GIF.

I just tested and you're correct, the issue only reproduces after clicking on the tab bar. Tried on both the .exe and .zip files for 64-bit Windows.

Note: in v3.4.1 the issue described in #334 occurs whether you've clicked on the webview or the tab bar, at least on 64-bit Windows installer. So this would be an improvement to it.

@razzeee
Copy link
Contributor Author

razzeee commented Oct 19, 2016

For reference electron/electron#7375

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 19, 2016

Thanks all! Yeah, #334 is improved compared to v3.4.1.
From the result, we have to try to keep the focus of webview as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants