fix(Dropdown): fix multiple problems #1795

layershifter · 2017-06-24T12:28:55Z

Fixes #1787.
Fixes #1858.
Codepen

Single select, placeholder

Click on placeholder doesn't capture the search input focus.

Expected behaviour

Single select, arrows

Click on up or down arrows resets current search value.

Current behaviour

Expected behaviour

Single select

Click on dropdown icon should always open the dropdown.

Current behaviour

Expected behaviour

Multi select, click

Click the dropdown (doesn't matter where) doesn't make anything.

Expected behaviour

codecov-io · 2017-06-24T12:35:40Z

Codecov Report

Merging #1795 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1795      +/-   ##
==========================================
+ Coverage   99.75%   99.75%   +<.01%     
==========================================
  Files         145      145              
  Lines        2477     2488      +11     
==========================================
+ Hits         2471     2482      +11     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Dropdown/Dropdown.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14fa291...21ec3cf. Read the comment docs.

layershifter · 2017-06-24T13:29:19Z

src/modules/Dropdown/Dropdown.js

+      this.open(e)
+      return
+    }
+    if (this.searchRef) this.searchRef.focus()


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

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?

Too many conditions for one method as I think.

layershifter · 2017-06-24T13:31:17Z

src/modules/Dropdown/Dropdown.js

+    _.invoke(this.props, 'onClick', e, this.props)
+    // prevent handleClick()
+    e.stopPropagation()
+    this.toggle(e)


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.

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.

layershifter · 2017-06-24T13:46:01Z

src/modules/Dropdown/Dropdown.js

@@ -848,12 +861,12 @@ export default class Dropdown extends Component {
  // Setters
  // ----------------------------------------

-  setValue = (value) => {
+  setValue = (value, resetQuery = true) => {


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.

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:

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 :/

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

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

layershifter · 2017-06-24T13:56:57Z

I performed required changes, in most cases the Dropdown is working as expected, but there are there some behaviours whose destiny should be solved before I will fix tests and add new.

@levithomason your review is very necessary, also I will be happy to hear @klerkendev there.

layershifter · 2017-06-29T07:40:35Z

I extracted RFCs to separate issues: #1806 and #1807.

…github.com/Semantic-Org/Semantic-UI-React into fix/dropdown-min-characters

layershifter · 2017-06-29T08:47:11Z

src/modules/Dropdown/Dropdown.js

+
+  handleIconOverrides = predefinedProps => ({
+    onClick: (e) => {
+      _.invoke(predefinedProps, 'onClick', e, predefinedProps)


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.

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 :)

layershifter · 2017-06-29T08:51:26Z

test/specs/commonTests/implementsShorthandProp.js

    requiredProps = {},
  } = options
  const { assertRequired } = helpers('implementsShorthandProp', Component)
+  const assertMethod = strictAssert ? 'contain' : 'containMatchingElement'


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.

layershifter

@levithomason we're ready for review there.

…React into fix/dropdown-min-characters # Conflicts: # test/specs/modules/Dropdown/Dropdown-test.js

layershifter · 2017-07-09T11:09:54Z

Requires #1845.

…React into fix/dropdown-min-characters # Conflicts: # test/specs/modules/Dropdown/Dropdown-test.js

levithomason · 2017-07-21T15:30:28Z

@layershifter #1845 has been merged, should be unblocked here.

levithomason

I've left some notes regarding splitting setValue, clearSearchQuery, and makeSelectedItemActive.

…ntic-Org/Semantic-UI-React into fix/dropdown-min-characters

…React into fix/dropdown-min-characters

layershifter

@levithomason I've performed requested changes

levithomason · 2017-07-29T17:21:25Z

This looks great, thanks for pressing through on this!

levithomason · 2017-07-29T18:57:50Z

Released in [email protected]

fix(Dropdown): fix multiple problems with minCharacters

0163591

layershifter commented Jun 24, 2017

View reviewed changes

layershifter force-pushed the fix/dropdown-min-characters branch from d6bed05 to 725941f Compare June 24, 2017 13:32

fix(Dropdown): fix multiple problems with minCharacters

642fe6a

layershifter force-pushed the fix/dropdown-min-characters branch from 725941f to 642fe6a Compare June 24, 2017 13:43

layershifter commented Jun 24, 2017

View reviewed changes

layershifter added the ready for review label Jun 24, 2017

layershifter requested a review from levithomason June 24, 2017 13:58

This was referenced Jun 29, 2017

RFC: Dropdown loses the focus on the search input #1806

Closed

RFC: Dropdown, selected item should be greyed out #1807

Closed

Merge branches 'fix/dropdown-min-characters' and 'master' of https://…

d547dd0

…github.com/Semantic-Org/Semantic-UI-React into fix/dropdown-min-characters

layershifter changed the title ~~WIP(Dropdown): fix multiple problems with minCharacters~~ fix(Dropdown): fix multiple problems Jun 29, 2017

layershifter commented Jun 29, 2017

View reviewed changes

test(Dropdown): add new tests for fixes

f6539f0

layershifter force-pushed the fix/dropdown-min-characters branch from 311d59c to f6539f0 Compare June 29, 2017 08:48

layershifter commented Jun 29, 2017

View reviewed changes

layershifter mentioned this pull request Jun 29, 2017

feat(Embed): add iframe shorthand #1810

Merged

layershifter added 3 commits July 9, 2017 13:51

test(shorthand): revert changes

2854e18

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

6c8b203

…React into fix/dropdown-min-characters # Conflicts: # test/specs/modules/Dropdown/Dropdown-test.js

style(Dropdown): fix lint issue

b51fd58

layershifter added blocked and removed ready for review labels Jul 9, 2017

This was referenced Jul 9, 2017

tests(shorthand): allow non-strict assert #1845

Merged

Dropdown: Filtered list resets when using arrow keys #1858

Closed

Alexander Fedyashov added 2 commits July 17, 2017 11:07

test(Dropdown): add new tests for fixes

b626d98

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

002f411

…React into fix/dropdown-min-characters # Conflicts: # test/specs/modules/Dropdown/Dropdown-test.js

layershifter force-pushed the fix/dropdown-min-characters branch from b51fd58 to 002f411 Compare July 17, 2017 08:09

levithomason added needs author feedback and removed blocked labels Jul 21, 2017

levithomason requested changes Jul 21, 2017

View reviewed changes

layershifter added 3 commits July 25, 2017 10:14

Merge branch 'fix/dropdown-min-characters' of https://github.com/Sema…

2fa2fbd

…ntic-Org/Semantic-UI-React into fix/dropdown-min-characters

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

a69584f

…React into fix/dropdown-min-characters

fix(Dropdown): more fixes

21ec3cf

layershifter added ready for review and removed needs author feedback labels Jul 25, 2017

layershifter commented Jul 25, 2017

View reviewed changes

levithomason approved these changes Jul 29, 2017

View reviewed changes

levithomason merged commit 62507c2 into master Jul 29, 2017

levithomason deleted the fix/dropdown-min-characters branch July 29, 2017 17:21

levithomason mentioned this pull request Aug 13, 2017

RFC: Multiple Search Dropdown -> not clearing search query on select #1954

Closed

This was referenced Aug 29, 2017

Multiple Search Selection Dropdown keeps input when selecting #2015

Closed

fix(Dropdown): input doesn't get cleared when adding item #2048

Closed

layershifter mentioned this pull request Sep 24, 2017

breaking(Dropdown): clean searchQuery after selection in multiple search Dropdown #2112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Dropdown): fix multiple problems #1795

fix(Dropdown): fix multiple problems #1795

layershifter commented Jun 24, 2017 •

edited

Loading

codecov-io commented Jun 24, 2017 •

edited

Loading

layershifter Jun 24, 2017

levithomason Jul 21, 2017

layershifter Jul 25, 2017

layershifter Jun 24, 2017

levithomason Jul 21, 2017

layershifter Jun 24, 2017

levithomason Jul 21, 2017

layershifter Jul 25, 2017

layershifter Jul 25, 2017

layershifter commented Jun 24, 2017

layershifter commented Jun 29, 2017

layershifter Jun 29, 2017

levithomason Jul 21, 2017

layershifter Jun 29, 2017

layershifter left a comment

layershifter commented Jul 9, 2017

levithomason commented Jul 21, 2017

levithomason left a comment

layershifter left a comment

levithomason commented Jul 29, 2017

levithomason commented Jul 29, 2017

fix(Dropdown): fix multiple problems #1795

fix(Dropdown): fix multiple problems #1795

Conversation

layershifter commented Jun 24, 2017 • edited Loading

Single select, placeholder

Expected behaviour

Single select, arrows

Current behaviour

Expected behaviour

Single select

Current behaviour

Expected behaviour

Multi select, click

Expected behaviour

codecov-io commented Jun 24, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

componentWillMount

componentWillReceiveProps

makeSelectedItemActive

removeItemOnBackspace

handleItemClick

handleLabelRemove

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Jun 24, 2017

layershifter commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter left a comment

Choose a reason for hiding this comment

layershifter commented Jul 9, 2017

levithomason commented Jul 21, 2017

levithomason left a comment

Choose a reason for hiding this comment

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Jul 29, 2017

levithomason commented Jul 29, 2017

layershifter commented Jun 24, 2017 •

edited

Loading

codecov-io commented Jun 24, 2017 •

edited

Loading