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

Allow ctrl + backspace to refocus the search #914

Merged
merged 1 commit into from
May 12, 2024

Conversation

Kuuuube
Copy link
Member

@Kuuuube Kuuuube commented May 11, 2024

#840 added support for refocusing on backspace but left out support for ctrl + backspace

@Kuuuube Kuuuube added kind/enhancement The issue or PR is a new feature or request area/ui-ux The issue or PR is related to UI/UX/Design labels May 11, 2024
@Kuuuube Kuuuube requested a review from a team as a code owner May 11, 2024 16:51
@@ -158,7 +158,7 @@ export class SearchDisplayController {
if (
activeElement !== this._queryInput &&
!this._isElementInput(activeElement) &&
!e.ctrlKey &&
(!e.ctrlKey || e.key === 'Backspace') &&
Copy link
Collaborator

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)

Copy link
Collaborator

@jamesmaa jamesmaa May 12, 2024

Choose a reason for hiding this comment

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

Suggested change
(!e.ctrlKey || e.key === 'Backspace') &&
!(e.ctrlKey && e.key === 'Backspace') &&

i think this is what you intended?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Collaborator

@jamesmaa jamesmaa May 12, 2024

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

@jamesmaa jamesmaa added this pull request to the merge queue May 12, 2024
Merged via the queue into themoeway:master with commit 9b28e8e May 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui-ux The issue or PR is related to UI/UX/Design kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants