-
-
Notifications
You must be signed in to change notification settings - Fork 479
Make searchbar more keyboard-friendly #1752
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
Conversation
aa0072c
to
2f62f04
Compare
c5a3079
to
ae81a21
Compare
This is ready to review on my side :) @fflorent @hexaltation if you wanna see |
Deployed commit |
) | ||
), | ||
dom.onKeyDown({ | ||
Escape$: () => { |
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.
Why is there a $ here?
Does it act like a regex or is it a typo for the key code Escape
?
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.
It's a grainjs feature:
By default, registered keyboard events are stopped from bubbling with stopPropagation() and preventDefault(). If, however, you register a key with a "$" suffix (i.e. Enter$ instead of Enter), then the event is allowed to bubble normally.
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.
But your question shows I should add a comment here explaining why we don't want to stopPropagation here, you are right it's not obvious 👀
edit: added a comment to clarify
ae81a21
to
022983b
Compare
Deployed commit |
Thanks for working on this @manuhabitela. The |
022983b
to
c22ce57
Compare
Woops, good catch thanks. I made a tiny change to make sure they work. |
Deployed commit |
Thanks for the quick fix. One other thing is that clicking the previous/next buttons or the search all pages checkbox no longer focuses the input. We have some tests not yet in this repo. Let me see about moving them here so you have more context. |
That one I kinda removed intentionally, felt like sticking with usual browser behavior was a good thing part of the cleanup. But I added it back with latest commit. This only applies when using the mouse in order to not mess up with keyboard nav where this behavior would be a bit weird :) |
00b2928
to
cfbebcc
Compare
Deployed commit |
cfbebcc
to
0d2f84e
Compare
Deployed commit |
Context
The document search bar in the top panel has a few issues:
Proposed solution
The idea behind this PR is:
Has this been tested?
I did not add any specific test ; the few current tests making searches still pass. Not sure taking time to carefully test behavior is a must add here, I can add some if you think we must :)
Screenshots / Screencasts
Before:
searchbar.before.mp4
After:
search.mp4