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(highlightedIndex): do not highlight disabled items by keyboard #799

Merged
merged 7 commits into from
Dec 31, 2019

Conversation

silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented Oct 22, 2019

What:

Allows disabled items to be skipped by keyboard highlight (arrow keys, home, end, character keys).

Why:

To have this scenario properly working.

How:

Refactored the utils methods that handle the index generation logic for highlighting. These methods also request getItemNodeFromIndex that will tell them if the item is marked as disabled or not.

This method comes from the hooks / component and is sent as parameter to the utils functions.

Test cases:

  • find next non-disabled if next is disabled on arrow down.

  • find prev non-disabled if prev is disabled on arrow up.

  • if not circular and no non-disabled item can be highlighted, index should stay the same.

  • if circular, the search for non-disabled item is resumed from first / last item.

  • on home/end, if first/last are disabled, then start searching downwards/upwards for first non-disabled.

  • on character key (useSelect) it will not highlight if item is disabled, it will search instead for the next item that starts with the key(s) and is not disabled.

Additional changes:

  • useSelect has false default prop for circularNavigation, while useCombobox has it `true.
  • unit tests in getItemProps for <Downshift>.
  • useCombobox now works properly with initialHighlightedIndex and defaultHighlightedIndex.
  • full testing for the getNextWrappingIndex and getItemIndexByCharacterKey

Checklist:

  • Documentation
  • Tests
  • TypeScript Types
  • Flow Types
  • Ready to be merged

Started to work based on #752

Closes #728 #825

@weyert
Copy link

weyert commented Dec 19, 2019

How can we progress this PR? Would it be possible to merge this PR with the logic for vanilla downshift and have the useSelect scenarios as enhancement?

@silviuaavram silviuaavram force-pushed the fix/not-highlight-disabled branch from b6325b4 to acd2427 Compare December 27, 2019 12:38
@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #799 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #799   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         886    923   +37     
  Branches      184    190    +6     
=====================================
+ Hits          886    923   +37
Impacted Files Coverage Δ
src/hooks/useSelect/reducer.js 100% <ø> (ø) ⬆️
src/utils.js 100% <100%> (ø) ⬆️
src/hooks/useCombobox/index.js 100% <100%> (ø) ⬆️
src/hooks/useSelect/index.js 100% <100%> (ø) ⬆️
src/hooks/useSelect/utils.js 100% <100%> (ø) ⬆️
src/hooks/utils.js 100% <100%> (ø) ⬆️
src/hooks/useCombobox/reducer.js 100% <100%> (ø) ⬆️
src/downshift.js 100% <100%> (ø) ⬆️
src/hooks/useCombobox/utils.js 100% <100%> (ø) ⬆️

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 e2404b7...7607445. Read the comment docs.

@silviuaavram
Copy link
Collaborator Author

@weyert we should be close with this. I only need to add tests to the hooks and finish the PR description.

@silviuaavram silviuaavram changed the title fix: do not highlight disabled items by keyboard fix(highlightedIndex): do not highlight disabled items by keyboard Dec 27, 2019
@silviuaavram silviuaavram changed the title fix(highlightedIndex): do not highlight disabled items by keyboard feat(highlightedIndex): do not highlight disabled items by keyboard Dec 27, 2019
@silviuaavram silviuaavram changed the title feat(highlightedIndex): do not highlight disabled items by keyboard fix(highlightedIndex): do not highlight disabled items by keyboard Dec 29, 2019
@weyert
Copy link

weyert commented Dec 29, 2019

Which tests need to be added? Looks like it already has 100% coverage?

@silviuaavram
Copy link
Collaborator Author

I was not 100% confident even with that coverage so added separate tests for the index generating function. Now it's better.

@weyert
Copy link

weyert commented Dec 30, 2019

Amazing, thanks @silviuavram :D

@silviuaavram silviuaavram merged commit 84b6ff6 into master Dec 31, 2019
@silviuaavram silviuaavram deleted the fix/not-highlight-disabled branch December 31, 2019 11:09
@silviuaavram
Copy link
Collaborator Author

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@weyert
Copy link

weyert commented Dec 31, 2019

Awesome! Thank you, excited about this 😀

@silviuaavram
Copy link
Collaborator Author

@all-contributors please add @jgodi for code

@allcontributors
Copy link
Contributor

@silviuavram

I've put up a pull request to add @jgodi! 🎉

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

Successfully merging this pull request may close these issues.

Skip selecting disabled items
3 participants