Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Dropdown): reset highlighted index on input change #1168

Merged
silviuaavram merged 13 commits into from
Apr 8, 2019
Merged

fix(Dropdown): reset highlighted index on input change #1168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6b96fa0
change highlightedIndex on input change
silviuaavram Apr 4, 2019
2591488
added unit tests for the change
silviuaavram Apr 4, 2019
56444dc
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 4, 2019
2104798
updated changelog
silviuaavram Apr 4, 2019
963f3f7
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 5, 2019
0006711
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 5, 2019
40e3ef9
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 5, 2019
347cb8c
integrated some code review comments
silviuaavram Apr 5, 2019
9e956d2
added new test for subsequent openings of dropdown
silviuaavram Apr 5, 2019
77ce9fd
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 5, 2019
6ae5c03
split some more tests
silviuaavram Apr 5, 2019
b61502c
Merge branch 'fix-highlighted-index-on-input-change' of https://githu…
silviuaavram Apr 5, 2019
5ff41cc
Merge branch 'master' into fix-highlighted-index-on-input-change
silviuaavram Apr 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Rename `flipInRtl` Icon's `slot` to `svgFlippingInRtl` in Teams theme @mnajdova ([#1179](https://github.com/stardust-ui/react/pull/1179))

### Fixes
- Fix the reset of the `highlightedIndex` when search query changes @silviuavram ([#1168](https://github.com/stardust-ui/react/pull/1168))
- Fix click triggering logic of `Space` and `Enter` keys for `MenuItem` @kuzhelov ([#1175](https://github.com/stardust-ui/react/pull/1175))
- Truncate `content` and `header` of `ListItem` when used from `DropdownSelectedItem` @silviuavram ([#1161](https://github.com/stardust-ui/react/pull/1161))
- Fix `rotate` prop on `Icon` not working in `rtl` @mnajdova ([#1179](https://github.com/stardust-ui/react/pull/1179))
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/components/Dropdown/Dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ class Dropdown extends AutoControlledComponent<Extendable<DropdownProps>, Dropdo
private handleSearchQueryChange = (searchQuery: string) => {
this.trySetStateAndInvokeHandler('onSearchQueryChange', null, {
searchQuery,
highlightedIndex: this.props.highlightFirstItemOnOpen ? 0 : null,
open: searchQuery === '' ? false : this.state.open,
})
}
Expand Down
167 changes: 158 additions & 9 deletions packages/react/test/specs/components/Dropdown/Dropdown-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react'
import * as keyboardKey from 'keyboard-key'

import Dropdown from 'src/components/Dropdown/Dropdown'
import DropdownSearchInput from 'src/components/Dropdown/DropdownSearchInput'
import { isConformant } from 'test/specs/commonTests'
import { mountWithProvider } from 'test/utils'

Expand Down Expand Up @@ -40,7 +41,7 @@ describe('Dropdown', () => {
onOpenChange.mockReset()
})

it('calls onOpen with highlightedIndex on click', () => {
it('has the provided prop value when opened by click', () => {
const highlightedIndex = 1
const wrapper = mountWithProvider(
<Dropdown highlightedIndex={highlightedIndex} onOpenChange={onOpenChange} items={items} />,
Expand All @@ -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', () => {
const highlightedIndex = 1
const wrapper = mountWithProvider(
<Dropdown highlightedIndex={highlightedIndex} onOpenChange={onOpenChange} items={items} />,
Expand All @@ -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', () => {
const highlightedIndex = 1
const wrapper = mountWithProvider(
<Dropdown highlightedIndex={highlightedIndex} onOpenChange={onOpenChange} items={items} />,
Expand All @@ -97,7 +98,7 @@ describe('Dropdown', () => {
)
})

it('calls onOpen with highlightedIndex wrapped to 0 on arrow down', () => {
it('is 0 when the provided prop value is last item index and opened by ArrowDown', () => {
const highlightedIndex = items.length - 1
const wrapper = mountWithProvider(
<Dropdown highlightedIndex={highlightedIndex} onOpenChange={onOpenChange} items={items} />,
Expand All @@ -117,7 +118,7 @@ describe('Dropdown', () => {
)
})

it('calls onOpen with highlightedIndex wrapped to items.length - 1 on arrow up', () => {
it('is last item index when the provided prop value is 0 and opened by ArrowUp', () => {
const highlightedIndex = 0
const wrapper = mountWithProvider(
<Dropdown highlightedIndex={highlightedIndex} onOpenChange={onOpenChange} items={items} />,
Expand All @@ -137,7 +138,7 @@ describe('Dropdown', () => {
)
})

it('calls onOpen with highlightedIndex set to defaultHighlightedIndex', () => {
it('is defaultHighlightedIndex prop value at first opening', () => {
const defaultHighlightedIndex = 1
const wrapper = mountWithProvider(
<Dropdown
Expand All @@ -158,9 +159,28 @@ describe('Dropdown', () => {
)
})

it('calls onOpen with highlightedIndex always set to 0', () => {
it('is null on second and subsequent open even though defaultHighlightedIndex prop is passed', () => {
const wrapper = mountWithProvider(
<Dropdown highlightFirstItemOnOpen={true} onOpenChange={onOpenChange} items={items} />,
<Dropdown defaultHighlightedIndex={1} onOpenChange={onOpenChange} items={items} />,
)
wrapper
Copy link
Contributor

@kuzhelov kuzhelov Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@kuzhelov kuzhelov Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@silviuaavram silviuaavram Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@kuzhelov kuzhelov Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 starts

If 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

Copy link
Collaborator Author

@silviuaavram silviuaavram Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the test into two.

.find({ className: Dropdown.slotClassNames.triggerButton })
.simulate('click')
.simulate('click')
.simulate('click')
expect(onOpenChange).toBeCalledTimes(3)
expect(onOpenChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
highlightedIndex: null,
open: true,
}),
)
})

it('is 0 on first open when highlightFirstItemOnOpen prop is provided', () => {
const wrapper = mountWithProvider(
<Dropdown highlightFirstItemOnOpen onOpenChange={onOpenChange} items={items} />,
)
const triggerButton = wrapper.find({ className: Dropdown.slotClassNames.triggerButton })

Expand All @@ -173,7 +193,18 @@ describe('Dropdown', () => {
open: true,
}),
)
triggerButton.simulate('click').simulate('click')
})

it('is 0 on second and subsequent open when highlightFirstItemOnOpen prop is provided', () => {
const wrapper = mountWithProvider(
<Dropdown highlightFirstItemOnOpen onOpenChange={onOpenChange} items={items} />,
)
const triggerButton = wrapper.find({ className: Dropdown.slotClassNames.triggerButton })

triggerButton
.simulate('click')
.simulate('click')
.simulate('click')
expect(onOpenChange).toBeCalledTimes(3)
expect(onOpenChange).toHaveBeenCalledWith(
null,
Expand All @@ -183,6 +214,124 @@ describe('Dropdown', () => {
}),
)
})

it('is 0 on searchQuery first change and when highlightFirstItemOnOpen prop is provided', () => {
const onSearchQueryChange = jest.fn()
const wrapper = mountWithProvider(
<Dropdown
search
highlightFirstItemOnOpen
onSearchQueryChange={onSearchQueryChange}
onOpenChange={onOpenChange}
items={items}
/>,
)
const searchInput = wrapper.find(`input.${DropdownSearchInput.slotClassNames.input}`)

searchInput.simulate('click').simulate('change', { target: { value: 'i' } })
expect(onOpenChange).toBeCalledTimes(1)
expect(onOpenChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
highlightedIndex: 0,
open: true,
}),
)
expect(onSearchQueryChange).toBeCalledTimes(1)
expect(onSearchQueryChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
searchQuery: 'i',
highlightedIndex: 0,
}),
)
})

it('is reset to 0 on searchQuery change and when highlightFirstItemOnOpen prop is provided', () => {
const onSearchQueryChange = jest.fn()
const wrapper = mountWithProvider(
<Dropdown
search
highlightFirstItemOnOpen
onSearchQueryChange={onSearchQueryChange}
onOpenChange={onOpenChange}
items={items}
/>,
)
const searchInput = wrapper.find(`input.${DropdownSearchInput.slotClassNames.input}`)

searchInput
.simulate('click')
.simulate('change', { target: { value: 'i' } })
.simulate('keydown', { keyCode: keyboardKey.ArrowUp, key: 'ArrowUp' }) // now it's on index 1.
.simulate('change', { target: { value: 'in' } }) // now it should reset to 0.
expect(onSearchQueryChange).toBeCalledTimes(2)
expect(onSearchQueryChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
searchQuery: 'in',
highlightedIndex: 0,
}),
)
})

it('is null on searchQuery first change and when highlightFirstItemOnOpen prop is not provided', () => {
const onSearchQueryChange = jest.fn()
const wrapper = mountWithProvider(
<Dropdown
search
onSearchQueryChange={onSearchQueryChange}
onOpenChange={onOpenChange}
items={items}
/>,
)
const searchInput = wrapper.find(`input.${DropdownSearchInput.slotClassNames.input}`)

searchInput.simulate('click').simulate('change', { target: { value: 'i' } })
expect(onOpenChange).toBeCalledTimes(1)
expect(onOpenChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
highlightedIndex: null,
open: true,
}),
)
expect(onSearchQueryChange).toBeCalledTimes(1)
expect(onSearchQueryChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
searchQuery: 'i',
highlightedIndex: null,
}),
)
})

it('is reset to null on searchQuery change and when highlightFirstItemOnOpen prop is not provided', () => {
const onSearchQueryChange = jest.fn()
const wrapper = mountWithProvider(
<Dropdown
search
onSearchQueryChange={onSearchQueryChange}
onOpenChange={onOpenChange}
items={items}
/>,
)
const searchInput = wrapper.find(`input.${DropdownSearchInput.slotClassNames.input}`)

searchInput
.simulate('click')
.simulate('change', { target: { value: 'i' } }) // no item highlighted.
.simulate('keydown', { keyCode: keyboardKey.ArrowUp, key: 'ArrowUp' }) // highlight on index 0.
.simulate('change', { target: { value: 'in' } })
expect(onSearchQueryChange).toBeCalledTimes(2)
expect(onSearchQueryChange).toHaveBeenCalledWith(
null,
expect.objectContaining({
searchQuery: 'in',
highlightedIndex: null,
}),
)
})
})

describe('getA11ySelectionMessage', () => {
Expand Down