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

fix: add proper tabindex when filterable #2334

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

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Feb 3, 2022

Currently, when a Select is filterable you cannot reach the filter via tab control. This PR will add a proper tabindex to the inputs.

@vercel
Copy link

vercel bot commented Feb 3, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/DBX6EhbUz9J8rrULW3nbJHneC7V6
✅ Preview: https://naive-ui-git-fork-jaulz-fix-add-proper-tabindex-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #2334 (dfeec06) into main (875b836) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2334      +/-   ##
==========================================
- Coverage   63.40%   63.39%   -0.02%     
==========================================
  Files         946      946              
  Lines       19261    19266       +5     
  Branches     4890     4893       +3     
==========================================
+ Hits        12212    12213       +1     
- Misses       5881     5885       +4     
  Partials     1168     1168              
Impacted Files Coverage Δ
src/_internal/selection/src/Selection.tsx 39.22% <37.50%> (-0.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 875b836...dfeec06. Read the comment docs.

@07akioni
Copy link
Collaborator

07akioni commented Feb 3, 2022

You can open select by pressing enter if it's focused.

I make lazy focus intentionally. Is the behavior problematic?

@jaulz
Copy link
Contributor Author

jaulz commented Feb 3, 2022

For normal selects it's fine but if the select is filterable I expect the input to be "tabbable" because I just want to type and select the input that matches my input. In my case I have a select that lets you pick your city from Google Places API. The options are requested on demand as the user types and there shouldn't be the possibility to enter free text.

@07akioni
Copy link
Collaborator

07akioni commented Feb 3, 2022

Can you rebase it? I want to see vercel preview.

@jaulz
Copy link
Contributor Author

jaulz commented Feb 3, 2022

@07akioni here you go 😊

@07akioni
Copy link
Collaborator

07akioni commented Feb 3, 2022

Normal select's tab focusing is broken.

@jaulz
Copy link
Contributor Author

jaulz commented Feb 3, 2022

Hm, what's not working for you? I can still focus the tab and press enter to open it.

@07akioni
Copy link
Collaborator

07akioni commented Feb 3, 2022

Tab focus this

image

@jaulz
Copy link
Contributor Author

jaulz commented Feb 24, 2022

@07akioni sorry for the issue I missed that before. I think now it works fine and whenever a select is filterable you can tab properly and can start immediately typing.

@07akioni
Copy link
Collaborator

After esc is pressed in filterable select, the select box should still be focused.

image

I think maybe provide a prop to let user choose whether to focus input immediately is better.

@jaulz
Copy link
Contributor Author

jaulz commented Mar 11, 2022

Okay, sure. Any suggestion for the name of the prop? 😊

@07akioni
Copy link
Collaborator

Okay, sure. Any suggestion for the name of the prop? 😊

Maybe focus-input-when-filterable? Not quite sure.

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

Successfully merging this pull request may close these issues.

2 participants