feat(Dropdown): highlight by character key in non-search #1270
Conversation
silviuaavram
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
silviuaavram
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1270 +/- ##
==========================================
+ Coverage 72.16% 72.32% +0.15%
==========================================
Files 762 762
Lines 5734 5766 +32
Branches 1678 1687 +9
==========================================
+ Hits 4138 4170 +32
Misses 1590 1590
Partials 6 6
Continue to review full report at Codecov.
|
silviuaavram
added
accessibility
silviuaavram
force-pushed
the
feat-highlight-by-letter-keys
branch
from
98ee868
to
d1b2257
Compare
| itemsList.simulate('keydown', { keyCode: keyboardKey.A, key: 'A' }) | ||
| expect(dropdown.state('highlightedIndex')).toBe(0) | ||
|
|
||
| jest.advanceTimersByTime(200) |
There was a problem hiding this comment.
nit: Dropdown.charKeyPressedCleanupTime / 2
| this.a11yStatusTimeout = setTimeout(() => { | ||
| this.setState({ a11ySelectionStatus: '' }) | ||
| }, Dropdown.a11yStatusCleanupTime) | ||
| } | ||
|
|
||
| private setStartingString = (startingString: string): void => { | ||
| clearTimeout(this.charKeysPressedTimeout) |
There was a problem hiding this comment.
I would replace clearTimeout + setTimeout with _.debounce
| const { value } = state | ||
|
|
||
| if (!items) { | ||
| return state |
There was a problem hiding this comment.
| return state | |
| return null |
getDerivedStateFromProps is invoked right before calling the render method, both on the initial mount and on subsequent updates. It should return an object to update the state, or null to update nothing.
https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops
| if (_.isFunction(search)) { | ||
| filteredResults = { | ||
| filteredItems: search(filteredItemsByValue, searchQuery), | ||
| } |
There was a problem hiding this comment.
As we have already used early return, I suggest to use it there, too
if (_.isFunction(search)) {
return { filteredItems: search(filteredItemsByValue, searchQuery) }
}There was a problem hiding this comment.
I thought it's better to have it assigned to that variable in branch, and return at the end {...state, ...filteredResults }. Now with return we need, on each branch, to return {...state, filteredItems: whateverResultOnTheBranch}.
Do you think it's better?
| @@ -1034,6 +1082,28 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo | |||
| } | |||
| } | |||
|
|
|||
| private getHighlightedIndexOnCharKeyDown = (keyString: string): number => { | |||
There was a problem hiding this comment.
I am lost with semantics of this method, it's called getHighlightedIndexOnCharKeyDown(), but it contains setStartingString
Should it be setHighlightedIndexOnCharKeyDown() and contain this.setState({ highlightedIndex })?
There was a problem hiding this comment.
good point. I will refactor it as a setter. Let's see if I also need to refactor the setStartingString one with debounce or not.
Behaves as in aria spec
When open, sending a character key will focus on the item that begins with that key.
If there is already an option highlighted, the search will begin from the next item.
If it reaches the end of the array and does not find any match, it will check from the start (circular movement).
If there is no option starting with that character, it will not do anything.
If characters are typed in rapid succession (one in under 500ms) it will search for the option starting with those keys. After 500ms after last key type, the 'starting with' string is cleared.
Also includes adding to the state the array of already filtered files (by value and searchQuery) to be used where needed. These strings will be updated only when the items are changed, value is changed in multiple selection or searchQuery is changed in search.