fix(Dropdown): reset highlighted index on input change #1168
Conversation
silviuaavram
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1168 +/- ##
==========================================
+ Coverage 82.36% 82.48% +0.11%
==========================================
Files 733 733
Lines 8716 8717 +1
Branches 1165 1232 +67
==========================================
+ Hits 7179 7190 +11
+ Misses 1522 1511 -11
- Partials 15 16 +1
Continue to review full report at Codecov.
|
| @@ -57,7 +58,7 @@ describe('Dropdown', () => { | |||
| ) | |||
| }) | |||
|
|
|||
| it('calls onOpen with highlightedIndex + 1 on arrow down', () => { | |||
| it('has the provided prop value + 1 when opened by ArrowDown', () => { | |||
There was a problem hiding this comment.
just for the sake of better clarity, lets use braces:
has value of (provided prop value + 1) when opened by ArrowDown
There was a problem hiding this comment.
sure, good idea!
| @@ -77,7 +78,7 @@ describe('Dropdown', () => { | |||
| ) | |||
| }) | |||
|
|
|||
| it('calls onOpen with highlightedIndex - 1 on arrow down', () => { | |||
| it('has the provided prop value - 1 when opened by ArrowUp', () => { | |||
There was a problem hiding this comment.
nit: lets try to avoid the terminology that is code specific, like ArrowUp - we might better use implementation-agnostic arrow up key
| @@ -137,7 +138,7 @@ describe('Dropdown', () => { | |||
| ) | |||
| }) | |||
|
|
|||
| it('calls onOpen with highlightedIndex set to defaultHighlightedIndex', () => { | |||
| it('is defaultHighlightedIndex value at first opening', () => { | |||
There was a problem hiding this comment.
nit-nit: just to be on par with the naming used fo rother test cases, lets add prop word, to better describe origin of defaultHighlightedIndex:
is defaultHighlightedIndex prop value at first opening
| @@ -156,11 +157,24 @@ describe('Dropdown', () => { | |||
| open: true, | |||
| }), | |||
| ) | |||
|
|
|||
| wrapper | |||
There was a problem hiding this comment.
actually, its not easy to see the reason for the event to happen this amount of times (which is requested by test). Could you, please, suggest why onOpenChange fires twice for each click?
There was a problem hiding this comment.
also, this check, apparently, verifies that on the subsequent open events defaultHighlightedIndex value is not honored. I would suggest to extract this check to the dedicated test case, with the corresponding name
There was a problem hiding this comment.
well, it is 3 because it opened a couple of times and closed once.
I just want to make sure that on opening it the second time, it opens and there is no item highlighted.
what do you think I should change?
There was a problem hiding this comment.
got it - I think that it would be more apparent if you would introduce a separate test case for that, the one that would follow a plan like:
- do whatever is necessary to prepare test state (arrange + act)
- verify conditions (assert)
Specifically to this example, it could be done the following way:
- introduce test case with a name like
is not set on second and subsequent openings - do the arrangement once there
wrapper
.find(...)
. ...
.simulate('click')
.expect( .. ) // and this is where assertion phase startsIf there will be just single arrangement phase followed by assertion, it would be much easier to infer where the numbers come from :) currently the main problem is that there are several such phases within single test, so that the side effect of the previous arrangement affects the following one, which is not intuitive and harder to maintain
There was a problem hiding this comment.
split the test into two.
| @@ -183,6 +197,95 @@ describe('Dropdown', () => { | |||
| }), | |||
| ) | |||
| }) | |||
|
|
|||
| it('is set to 0 at searchQuery change and when highlightFirstItemOnOpen prop is provided', () => { | |||
There was a problem hiding this comment.
nit: at -> on:
is set to 0 on searchQuery change and when highlightFirstItemOnOpen prop is provided
…b.com/stardust-ui/react into fix-highlighted-index-on-input-change
This should reset the
highlightedIndexto null or 0, depending whetherhighlightFirstItemOnOpenhas been passed or not.