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.

Sign up for GitHub

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

Jump to bottom

fix(Dropdown): fix multiple problems #1795

Merged
merged 12 commits into from
Jul 29, 2017
Merged

fix(Dropdown): fix multiple problems #1795

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0163591
fix(Dropdown): fix multiple problems with minCharacters
layershifter Jun 24, 2017
642fe6a
fix(Dropdown): fix multiple problems with minCharacters
layershifter Jun 24, 2017
d547dd0
Merge branches 'fix/dropdown-min-characters' and 'master' of https://…
Jun 29, 2017
f6539f0
test(Dropdown): add new tests for fixes
Jun 29, 2017
2854e18
test(shorthand): revert changes
layershifter Jul 9, 2017
6c8b203
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
layershifter Jul 9, 2017
b51fd58
style(Dropdown): fix lint issue
layershifter Jul 9, 2017
b626d98
test(Dropdown): add new tests for fixes
Jul 17, 2017
002f411
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
Jul 17, 2017
2fa2fbd
Merge branch 'fix/dropdown-min-characters' of https://github.com/Sema…
layershifter Jul 25, 2017
a69584f
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
layershifter Jul 25, 2017
21ec3cf
fix(Dropdown): more fixes
layershifter Jul 25, 2017
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,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 @@ -550,7 +550,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 @@ -568,10 +568,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 @@ -650,7 +650,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 @@ -848,12 +861,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 @@ -949,6 +962,17 @@ export default class Dropdown extends Component {
this.scrollSelectedItemIntoView()
}

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

handleIconOverrides = predefinedProps => ({
onClick: (e, props) => {
_.invoke(predefinedProps, 'onClick', e, props)
this.handleIconClick(e)
},
})

// ----------------------------------------
// Refs
// ----------------------------------------
Expand Down Expand Up @@ -1260,7 +1284,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