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

feat: Focus first match on search #370

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MJRuskin
Copy link
Contributor

@MJRuskin MJRuskin commented May 6, 2020

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.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • Updated documentation (if applicable)
  • Added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

@MJRuskin
Copy link
Contributor Author

MJRuskin commented May 6, 2020

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:

  • "correct" the original tests and create an additional test to demonstrate the new feature
    or
  • integrate a check for this new feature into the original two tests and correct the outcome

(I am assuming the second option is more preferable)

@mrchief
Copy link
Collaborator

mrchief commented May 6, 2020

Hey! Thanks, for sending this.

When searching, if the current focus is not a match, it will focus the first match.

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.

@MJRuskin
Copy link
Contributor Author

MJRuskin commented May 6, 2020

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.

@mrchief
Copy link
Collaborator

mrchief commented May 6, 2020

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.

@mrchief
Copy link
Collaborator

mrchief commented May 12, 2020

Can we also add some unit tests for this?

@MJRuskin
Copy link
Contributor Author

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.

@coveralls
Copy link

coveralls commented May 21, 2020

Pull Request Test Coverage Report for Build 1531

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 93.812%

Totals Coverage Status
Change from base Build 1526: -0.09%
Covered Lines: 603
Relevant Lines: 624

💛 - Coveralls

@mrchief mrchief added the wip Work In Progress label May 25, 2020
@codeclimate
Copy link

codeclimate bot commented Jun 18, 2020

Code Climate has analyzed commit df68ee2 and detected 0 issues on this pull request.

View more on Code Climate.

@marceleq27
Copy link

Hi, any updates in this PR? I was searching in docs for this feature and I came here :)

@mrchief
Copy link
Collaborator

mrchief commented Feb 6, 2021

@marceleq27 haven't had time to dig into this. Will try to get it this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants