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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Electron to zoom with CommandOrControl+= #8381

Merged
merged 2 commits into from Feb 7, 2019

Conversation

@aaronraimist
Copy link
Contributor

aaronraimist commented Feb 2, 2019

Fixes #7721

This creates two menu items which looks bad but setting the second one to visible: false seems to disable it too. 馃槨

screen shot 2019-02-02 at 3 55 48 pm

Hopefully someone will actually solve electron/electron#15496 upstream

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@jryans jryans requested a review from vector-im/riot-web Feb 4, 2019
Copy link
Member

jryans left a comment

I agree it's a bit sad that this is needed... Would it be a bit cleaner to listen for the other shortcut manually without a menu item?

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Feb 5, 2019

I don't know a lot about Electron but it seems like you can't do this very well. I guess you can do
electron/electron#1334 (comment) but I can't get it to work. electron-localshortcut also exists but also doesn't seem great. Electron itself only really supports creating global (affects the app even when the user isn't using the app) shortcuts without a menu item.

Given the workarounds above, we believe this module is better implemented in userland (ex. the excellent electron-localshortcut and thus i'm going to label this a wontfix.

electron/electron#1334 (comment)

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Feb 5, 2019

The other alternative is to get rid of the Shift+Command+Plus shortcut, the menu item will still look "wrong" but at least there will only be one

@jryans

This comment has been minimized.

Copy link
Member

jryans commented Feb 6, 2019

I'd prefer to find a way to have only one menu item with 鈱+ displayed, while still accepting both Cmd-Plus and Cmd-Shift-Plus, as that's how most other native apps appear to handle this issue.

Looking at Atom, that's what they seem to do, but of course their version is much more complex since they support user remappable shortcuts.

It's a bit sad that this is so hard to solve with Electron... 馃槗

@jryans

This comment has been minimized.

Copy link
Member

jryans commented Feb 6, 2019

The other alternative is to get rid of the Shift+Command+Plus shortcut, the menu item will still look "wrong" but at least there will only be one

Maybe that's an okay compromise, since it seems unreasonably hard to do the right thing in Electron.

I don't think anyone ever types Shift-Command-Plus anyway, so at least that would reflect reality.

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@jryans
jryans approved these changes Feb 7, 2019
Copy link
Member

jryans left a comment

Thanks, this looks good to me! 馃榿

@jryans jryans merged commit 5030410 into vector-im:develop Feb 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aaronraimist aaronraimist deleted the aaronraimist:electron-browser-zoom+ branch Feb 7, 2019
aaronraimist added a commit to aaronraimist/riot-web that referenced this pull request May 9, 2019
鈥 in vector-im#8381)

Signed-off-by: Aaron Raimist <aaron@raim.ist>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.