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

Change search hotkey so it does not replace the native find #2078

Merged
merged 2 commits into from
May 14, 2020

Conversation

belcherj
Copy link
Contributor

Fix

Change the search hotkey to ctrl + shift + s so that it does not conflict with the native browser find.

Test

  1. Goto app
  2. Use cmd/ctrl + f, does it open browser find?
  3. Use cmd/ctrl + shift + s, does it focus search field?

@belcherj belcherj self-assigned this May 12, 2020
@codebykat codebykat self-requested a review May 13, 2020 10:52
Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

I think we need to remap Ctrl-G too. Right now you can get into a state where you have two different searches in the app search and browser find dialog, and it's confusing. Like from here if I click in the app and use Ctrl-G, it cycles through "tags", but using Ctrl-F and then Ctrl-G makes it cycle through "trash". It's bad.

Screen Shot 2020-05-13 at 3 49 59 AM

Our Ctrl-G only exists because we were trying to take over the browser-based search entirely, but in the browser, it makes things worse. There's no reason to have it when it does the same thing as the browser's Find.

In Electron I really think having Ctrl-F and Ctrl-G is really nice. There's no built-in Find functionality so it makes sense there. Can we consider dropping these shortcuts only for the browser? We could ensure a nice cross-platform experience if we support Ctrl+Shift+S in Electron as well as Ctrl+F.

Overall I'm concerned about this entire search thing, it feels like we made a pretty major change on a whim when we should've looped more people into the discussion, and now we're reacting very quickly to feedback in a kind of piecemeal way (without a larger vision of how all this is supposed to fit together and ALSO without taking the time to think through all the flows).

@belcherj belcherj force-pushed the update/search-hotkey branch from f8e7194 to 0148881 Compare May 14, 2020 02:21
@codebykat codebykat self-requested a review May 14, 2020 06:48
Copy link
Member

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

Works great on OSX Firefox and Chrome. However, we have a shortcut conflict with Firefox on Windows, sigh... Ctrl+shift+S takes a screenshot 🙄.

That being said, I think this is what the shortcut was set to, pre-1.16? Maybe @dmsnell can confirm. If so, I would say let's run with it (AKA revert to what it was originally) and we can rethink from there.

Interestingly, I just tested in Parallels and Ctrl+F brings up the browser Find dialog anyway on Edge. o_O (Firefox behaves as expected.)

@belcherj
Copy link
Contributor Author

Grrrr, thought i tested all the browsers. There was no search before.

Screenshotting still works so I guess Firefox just wont get a hotkey. There are basically no hotketys left to use.

@belcherj belcherj merged commit 69e5085 into develop May 14, 2020
@belcherj belcherj deleted the update/search-hotkey branch May 14, 2020 13:41
@codebykat codebykat added this to the 1.17 milestone May 18, 2020
@codebykat codebykat mentioned this pull request May 20, 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