-
Notifications
You must be signed in to change notification settings - Fork 268
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
feat: Focus first match on search #370
base: main
Are you sure you want to change the base?
Conversation
Just realised this PR changes how some of the navigation tests would work - since the first match is automatically focused before navigating with the arrow keys. For fixing the tests, would you prefer me to:
(I am assuming the second option is more preferable) |
Hey! Thanks, for sending this.
So the reason we do not focus anything right now is to let the user keep on searching. It'd be annoying if I'm typing something and I pause for a sec and then focus is lost from the input. I'm not sure automatically switching focus is a good idea in that case but I'm happy to hear what your thoughts are. |
The search input will never lose focus. This change affects which item in the tree is highlighted. My intention for the change was to allow users to type and then simply press the Enter key to quickly select the first match. In the current state this requires the additional press of an arrow key to snap to the first match. I can gladly make this an optional feature instead. I just felt the current implementation was inconsistent when it came to navigation - especially when going back and forth between searching and the normal view. |
OK, I see. Yeah, it seems like this will be a good addition. I need to play with it a bit to see it in action but would love to hear what @ellinge thinks as well. |
Can we also add some unit tests for this? |
Just added some tests and fixed the ones that were affected by this change. I have never used enzyme before, so please let me know if something needs fixing. |
Pull Request Test Coverage Report for Build 1531
💛 - Coveralls |
Code Climate has analyzed commit df68ee2 and detected 0 issues on this pull request. View more on Code Climate. |
Hi, any updates in this PR? I was searching in docs for this feature and I came here :) |
@marceleq27 haven't had time to dig into this. Will try to get it this weekend. |
What does it do?
When searching, if the current focus is not a match, it will focus the first match.
Fixes # (issue)
No issueNumber
Type of change
Please delete options that are not relevant.
Checklist: