-
Notifications
You must be signed in to change notification settings - Fork 94
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
Allow ctrl + backspace to refocus the search #914
Conversation
@@ -158,7 +158,7 @@ export class SearchDisplayController { | |||
if ( | |||
activeElement !== this._queryInput && | |||
!this._isElementInput(activeElement) && | |||
!e.ctrlKey && | |||
(!e.ctrlKey || e.key === 'Backspace') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following this. If e.ctrlKey
and e.key === 'Backspace'
were both true, then this would be (!true || true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(!e.ctrlKey || e.key === 'Backspace') && | |
!(e.ctrlKey && e.key === 'Backspace') && |
i think this is what you intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean (e.ctrlKey && e.key === 'Backspace')
? But regardless it does the same thing.
(!true || true)
= true
(true && true)
= true
And if you actually did mean to put that ! there... !(true && true)
= false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looking at the entire if statement instead of this little chunk of it, none of these suggestions make sense and they just break the thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding proposal that doesn't work for documentation purposes. This would break existing behavior:
const isIgnoreFocusKey = e.ctrlKey || e.metaKey || e.altKey || e.key === ' ';
const isTriggerFocusKey = (e.ctrlKey && e.key === 'Backspace') || (e.key.length === 1 && e.key === 'Backspace')
if (activeElement !== this._queryInput && !this._isElementInput(activeElement) && (!isIgnoreFocusKey || isTriggerFocusKey)) {
...
}
Further discussion https://discord.com/channels/617136488840429598/1081538711742844980/1239039202805547049
#840 added support for refocusing on backspace but left out support for
ctrl + backspace