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(Dropdown): fix multiple problems #1795

Merged
merged 12 commits into from
Jul 29, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
44 changes: 35 additions & 9 deletions src/modules/Dropdown/Dropdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ export default class Dropdown extends Component {
if (move === undefined) return
e.preventDefault()
this.moveSelectionBy(move)
if (!multiple) this.makeSelectedItemActive(e)
if (!multiple) this.makeSelectedItemActive(e, false)
}

openOnSpace = (e) => {
Expand All @@ -559,7 +559,7 @@ export default class Dropdown extends Component {
this.open(e)
}

makeSelectedItemActive = (e) => {
makeSelectedItemActive = (e, resetQuery = true) => {
const { open } = this.state
const { multiple, onAddItem } = this.props
const item = this.getSelectedItem()
Expand All @@ -577,10 +577,10 @@ export default class Dropdown extends Component {
if (multiple) {
// state value may be undefined
const newValue = _.union(this.state.value, [value])
this.setValue(newValue)
this.setValue(newValue, resetQuery)
this.handleChange(e, newValue)
} else {
this.setValue(value)
this.setValue(value, resetQuery)
this.handleChange(e, value)
}
}
Expand Down Expand Up @@ -659,7 +659,20 @@ export default class Dropdown extends Component {

if (!search) return this.toggle(e)
if (open) return
if (searchQuery.length >= minCharacters || minCharacters === 1) this.open(e)
if (searchQuery.length >= minCharacters || minCharacters === 1) {
this.open(e)
return
}
if (this.searchRef) this.searchRef.focus()
Copy link
Member Author

Choose a reason for hiding this comment

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

@levithomason this method does not make me happy, do you have ideas how we can refactor it?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I don't think so. React has no focus mechanism so we'll have to use a ref or querySelector here. Since this section of logic is only reachable if this is a closed search dropdown, I think it makes sense.

What specifically are you not happy with and perhaps I can recommend better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Too many conditions for one method as I think.

}

handleIconClick = e => {
debug('handleIconClick()', e)

_.invoke(this.props, 'onClick', e, this.props)
// prevent handleClick()
e.stopPropagation()
this.toggle(e)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to split this to separate method, however I don't see any other way. The initial idea was to compare e.target in handleClick this the Icon's ref, but the Icon component is stateless and doesn't have refs.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I'd much rather have a separate explicit click handler here. This is OK. We are doing something entirely different when the icon is clicked.

}

handleItemClick = (e, item) => {
Expand Down Expand Up @@ -859,12 +872,12 @@ export default class Dropdown extends Component {
// Setters
// ----------------------------------------

setValue = (value) => {
setValue = (value, resetQuery = true) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that the idea with resetQuery is the best possible, hovewer I don't see another path, as I see in all other cases searchQuery should be reset.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should split this into two methods. setValue only sets the value and clearSearchQuery only clears the query. It is easier to test and reason about complex interactions when the methods are small and explicit.

There are also enough use cases to warrant seprating them I think:

componentWillMount

This calls setValue, however, it calls it with this.state.value. This is a redundant operation. It seems the only reason we call it is for the other logic in setValue, clearing the search query and setting the selected index.

We don't need to clear the query on mount, but we do need to set the selected index. I think this is duplicating work, but at the least, we don't need to clear the query on mount.

componentWillReceiveProps

This calls setValue when a new value is received. Though this is correct, I also don't think we want to clear the user's query when this happens. If a new value comes down while a query is entered, I think it should only change the value, not the query.

makeSelectedItemActive

This method currently takes a resetQuery argument and passes it down to setValue. I think this method should only make the selected item active and not do anything else. The call sites for this method should be responsible for also deciding to clear the query or not:

  • moveSelectionOnKeyDown - should never clear the query
  • selectItemOnEnter - should only clear query when !multiple
  • handleBlur - should clear the query (it is already only called when !multiple)

removeItemOnBackspace

Currently resets the query but is not necessary, there is no query at this point. Also, if the user enters a query and moves the cursor to the begining of the query and then presses backspace, we wouldn't want to clear the query either:

[ LabelA x ] [ LabelB x ] | My query
                          ^ cursor, backspace should not clear the query

I know we don't support this yet, but we technically could/should in the future.

handleItemClick

This currently resets the query when an menu item is made active, however, I have always found this to be a usability issue with multiple dropdowns. When there are lots of items, it is often more helpful to keep the query while clicking items. Otherwise, the user has to enter the same query over and over for every item. Example, selecting all states that start with ma:

http://g.recordit.co/OqqnZDHR5u.gif

If we separate setValue and clearSearchQuery, then we can call only setValue in the multiple condition in this method. The other branch (not multiple) we can safely clear the query becuase we know the user is done making selections.

handleLabelRemove

This will currently clear the query also but probably never should have done so. Here I'm again selecting states with ma. I "accidentally" select Alabama and continue making selections. I then remove Alabama while I have a query entered and my query gets removed :/

http://g.recordit.co/IqdAPXNVrQ.gif

Copy link
Member Author

Choose a reason for hiding this comment

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

@levithomason Thanks for a detailed feedback, I'm fully agree with your suggestions. Will make a changes soon

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we should extract behaviour of removeItemOnBackspace to separate issue, I fully agree that we need to impliment it.

debug('setValue()')
debug('value', value)
const newState = {
const newState = resetQuery ? {
searchQuery: '',
}
} : {}

const { multiple, search } = this.props
if (multiple && search && this.searchRef) this.searchRef.focus()
Expand Down Expand Up @@ -960,6 +973,17 @@ export default class Dropdown extends Component {
this.scrollSelectedItemIntoView()
}

// ----------------------------------------
// Overrides
// ----------------------------------------

handleIconOverrides = predefinedProps => ({
onClick: (e) => {
_.invoke(predefinedProps, 'onClick', e, predefinedProps)
Copy link
Member Author

Choose a reason for hiding this comment

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

Please take an attention to this line, Icon component doesn't have onClick handler, so it doesn't have second argument. There are two possible solutions: remove the second argument or add onClick hanlder to the Icon component.

Copy link
Member

Choose a reason for hiding this comment

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

I see the issue here, however, I think this is going to be rare enough of an edge case to leave as-is for now. We shall see :)

this.handleIconClick(e)
},
})

// ----------------------------------------
// Refs
// ----------------------------------------
Expand Down Expand Up @@ -1275,7 +1299,9 @@ export default class Dropdown extends Component {
{this.renderSearchInput()}
{this.renderSearchSizer()}
{trigger || this.renderText()}
{Icon.create(icon)}
{Icon.create(icon, {
overrideProps: this.handleIconOverrides,
})}
{this.renderMenu()}
</ElementType>
)
Expand Down
9 changes: 6 additions & 3 deletions test/specs/commonTests/implementsShorthandProp.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ const shorthandComponentName = ShorthandComponent => {
* @param {object} options
* @param {string} options.propKey The name of the shorthand prop.
* @param {string|function} options.ShorthandComponent The component that should be rendered from the shorthand value.
* @param {function} options.mapValueToProps A function that maps a primitive value to the Component props
* @param {boolean} [options.alwaysPresent] Whether or not the shorthand exists by default.
* @param {function} options.mapValueToProps A function that maps a primitive value to the Component props.
* @param {Object} [options.requiredProps={}] Props required to render the component.
* @param {Object} [options.shorthandDefaultProps] Default props for the shorthand component.
* @param {Object} [options.shorthandOverrideProps] Override props for the shorthand component.
* @param {boolean} [options.alwaysPresent] Whether or not the shorthand exists by default
* @param {boolean} [options.strictAssert] Selects an assertion method, `contain` will be used if true.
*/
export default (Component, options = {}) => {
const {
Expand All @@ -32,9 +33,11 @@ export default (Component, options = {}) => {
ShorthandComponent,
shorthandDefaultProps = {},
shorthandOverrideProps = {},
strictAssert = true,
requiredProps = {},
} = options
const { assertRequired } = helpers('implementsShorthandProp', Component)
const assertMethod = strictAssert ? 'contain' : 'containMatchingElement'
Copy link
Member Author

Choose a reason for hiding this comment

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

Why? contains is too strict while I added onClick handler to the Icon it was real pain. I think it's a quite elegant solution, as I remember we had same problem with the Input component.


describe(`${propKey} shorthand prop (common)`, () => {
assertRequired(Component, 'a `Component`')
Expand All @@ -50,7 +53,7 @@ export default (Component, options = {}) => {
})
const element = createElement(Component, { ...requiredProps, [propKey]: value })

shallow(element).should.contain(shorthandElement)
shallow(element).should[assertMethod](shorthandElement)
}

if (alwaysPresent || Component.defaultProps && Component.defaultProps[propKey]) {
Expand Down
57 changes: 56 additions & 1 deletion test/specs/modules/Dropdown/Dropdown-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,16 @@ describe('Dropdown', () => {
common.isConformant(Dropdown)
common.hasUIClassName(Dropdown)
common.hasSubComponents(Dropdown, [DropdownDivider, DropdownHeader, DropdownItem, DropdownMenu, DropdownSearchInput])
common.implementsIconProp(Dropdown)

common.implementsIconProp(Dropdown, {
assertExactMatch: false,
})
common.implementsShorthandProp(Dropdown, {
propKey: 'header',
ShorthandComponent: DropdownHeader,
mapValueToProps: val => ({ content: val }),
})

common.propKeyOnlyToClassName(Dropdown, 'disabled')
common.propKeyOnlyToClassName(Dropdown, 'error')
common.propKeyOnlyToClassName(Dropdown, 'loading')
Expand Down Expand Up @@ -431,6 +435,35 @@ describe('Dropdown', () => {
wrapperRender(<Dropdown />)
.should.contain.descendants('.dropdown.icon')
})

it('always opens a dropdown on click', () => {
wrapperMount(<Dropdown options={options} selection search />)
.find('i.icon')
.simulate('click')

dropdownMenuIsOpen()
})

it('always opens a dropdown on click', () => {
wrapperMount(<Dropdown options={options} selection search />)
.find('i.icon')
.simulate('click')

dropdownMenuIsOpen()
})

it('passes onClick handler', () => {
const event = { target: null }
const onClick = sandbox.spy()
const props = { name: 'user', onClick }

wrapperMount(<Dropdown icon={props} options={options} />)
.find('i.icon')
.simulate('click', event)

onClick.should.have.been.calledOnce()
onClick.should.have.been.calledWithMatch(event, props)
})
})

describe('selected item', () => {
Expand Down Expand Up @@ -728,6 +761,16 @@ describe('Dropdown', () => {
domEvent.keyDown(document, { key: 'Enter' })
dropdownMenuIsClosed()
})

it('keeps value of the searchQuery when selection is changed', () => {
wrapperMount(<Dropdown options={options} selection search />)

wrapper.setState({ searchQuery: 'foo' })
wrapper.simulate('click')
domEvent.keyDown(document, { key: 'ArrowDown' })

wrapper.should.have.state('searchQuery', 'foo')
})
})

describe('value', () => {
Expand Down Expand Up @@ -1638,6 +1681,18 @@ describe('Dropdown', () => {
)
})

it('sets focus to the search input on click on the placeholder', () => {
wrapperMount(<Dropdown minCharacters={3} options={options} placeholder='foo' selection search />)
.find('.default.text')
.simulate('click')

const activeElement = document.activeElement
const searchIsFocused = activeElement === document.querySelector('input.search')
searchIsFocused.should.be.true(
`Expected "input.search" to be the active element but found ${activeElement} instead.`
)
})

it('clears the search query when an item is selected', () => {
// search for random item
const searchQuery = _.sample(options).text
Expand Down