[Hotkeys] Hide hotkeys dialog on 'shift + ?' keypress #1301
Conversation
Hide dialog after delay on 'shift + ?' pressPreview: documentation |
|
@cmslewis this is working so nicely, thanks taking care of this 🏆 |
| @@ -30,7 +30,8 @@ class HotkeysDialog { | |||
| private container: HTMLElement; | |||
| private hotkeysQueue = [] as IHotkeyProps[][]; | |||
| private isDialogShowing = false; | |||
| private timeoutToken = 0; | |||
| private showTimeoutToken = 0; | |||
| private hideTimeoutToken = 0; | |||
There was a problem hiding this comment.
feels more accurate to declare these as number rather than initialize to a likely invalid ID (although clearTimeout happily ignores invalid IDs)
| return; | ||
| } | ||
|
|
||
| const combo = getKeyCombo(e); | ||
|
|
||
| if (comboMatches(parseKeyCombo(SHOW_DIALOG_KEY), combo)) { | ||
| showHotkeysDialog(this.actions.map((action) => action.props)); | ||
| if (isHotkeysDialogShowing()) { | ||
| hideHotkeysDialogAfterDelay(); |
There was a problem hiding this comment.
why is the delay necessary here? what's wrong with simply hideHotkeysDialog?
There was a problem hiding this comment.
Because a bunch of disjoint hotkey keydown listeners will fire in succession. If you synchronously call hideHotkeysDialog in any one of them, then immediate successive calls to isHotkeysDialogShowing would return false, causing the hotkeys dialog to reopen (because the same hotkey that opens that dialog is now overloaded to close it too).
Introducing a small delay is the only way to guarantee all those keydown listeners execute before the dialog actually gets hidden.
Respond to CR feedbackPreview: documentation | table |
|
@giladgray can you take another look at this so we can get it merged? |
Fix buildPreview: documentation | table |
| /** | ||
| * Since one hotkey toggles the dialog open and closed, we need to | ||
| * introduce a delay to ensure that all hotkey listeners fire with the | ||
| * dialog in a consistent state. |
There was a problem hiding this comment.
this is not a /** */ doc comment; it's a code comment explaining why we must do it this way. i'd move it inside the function body and use //.
There was a problem hiding this comment.
could also just edit this text so it's useful to a consumer, to explain why you'd want to call this function instead of regular hide().
Move function commentPreview: documentation | table |
Fixes #1238
Checklist
Include tests(couldn't write a passing test for the life of me) :/Changes proposed in this pull request:
If the hotkeys dialog is visible, close it when shift + ? is pressed. That hotkey now acts as a toggle.