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

Electron: Handle all keyboard shortcuts that are in the menu, from the menu #2370

Merged
merged 3 commits into from
Sep 30, 2020

Conversation

codebykat
Copy link
Member

Fix

When we add menu items in Electron we're passing a keyboard shortcut, but we were clobbering those with our keyboard event listeners in the app. This causes them to fire and be handled without the Electron menu flashing, which makes things feel kind of janky and breaks the illusion of a native app.

This also fixes the missing handler for Find Note, although it does the exact same thing as Search in Notes, so it's kind of odd to have them both in the menu?

Test

  1. Go through the keyboard shortcuts and make sure they all work
  2. For the ones that are in the menu, verify that:
    a) they can be triggered by clicking in the menu
    b) triggering them by keyboard shortcut causes the menu to flash
  3. Make sure keyboard shortcuts also work in the browser

@codebykat codebykat added this to the Rewrite/beta milestone Sep 30, 2020
@codebykat codebykat requested a review from a team September 30, 2020 04:26
@codebykat codebykat self-assigned this Sep 30, 2020
@codebykat codebykat mentioned this pull request Sep 30, 2020
3 tasks
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

Tested and works well

@codebykat codebykat merged commit aebfc22 into develop Sep 30, 2020
@codebykat codebykat deleted the fix/electron-menus-and-shortcuts branch September 30, 2020 20:28
@codebykat codebykat modified the milestones: Rewrite/beta, 2.0.0 Oct 5, 2020
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.

2 participants