fix(Dropdown): fix `minCharacter` prop behaviour #1722

layershifter · 2017-05-31T16:15:41Z

Behaviours

This PR fixes issues with minCharacters and also some other inconsistent things.

TODO

onFocus (placeholder #1299, will be done in separate task)

onClick

Note, that if you click on input after it was openned, you will not close it, e.g. click don't toggle searchable dropdown.

`minCharacters` = 1

`minCharacters` = 3

Dropdown wasn't openned after click.

onFocus

`minCharacters` = 1

Note, focus doesn't affect placeholder. After focus was lost first item was selected, because item was focused.

`minCharacters` = 1

`minCharacters` = 3

Dropdown wasn't openned after click.

onChange

Note, is you have no results message and you click Enter key, nothing will be happened.

`minCharacters` = 1

`minCharacters` = 3

levithomason · 2017-06-01T05:47:40Z

docs/app/Examples/elements/Container/Types/ContainerExampleContainer.js

-  <Container>
-    <p>Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa strong. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. Nullam dictum felis eu pede link mollis pretium. Integer tincidunt. Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, viverra quis, feugiat a, tellus. Phasellus viverra nulla ut metus varius laoreet. Quisque rutrum. Aenean imperdiet. Etiam ultricies nisi vel augue. Curabitur ullamcorper ultricies nisi.</p>
-  </Container>
+const DropdownExampleSearchSelection = () => (


Whoops, this looks like the wrong content for this file.

Whoops, looks like I missed the big fat WIP 😊

Yep, I will revert this before merge 😄

codecov-io · 2017-06-01T14:25:37Z

Codecov Report

Merging #1722 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1722      +/-   ##
==========================================
- Coverage   99.75%   99.75%   -0.01%     
==========================================
  Files         142      142              
  Lines        2468     2464       -4     
==========================================
- Hits         2462     2458       -4     
  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 62b0fd6...5f70d73. Read the comment docs.

layershifter · 2017-06-02T16:01:09Z

@levithomason I done everything except placeholder's behaviour. Let me know what you think about proposed changes in this PR, gifs were captured from SUI, you can compare them with our current docs. If it will okay, I'll add tests and prepare PR for merge.

layershifter · 2017-06-02T16:01:27Z

src/modules/Dropdown/Dropdown.js

    noResultsMessage: 'No results found.',
+    openOnFocus: true,


Only prop sort there.

layershifter · 2017-06-02T16:02:11Z

src/modules/Dropdown/Dropdown.js

        debug('mouse is not down, opening')
-        if (openOnFocus) this.open()
+        if (openOnFocus && openable) this.open()


Defines behaviour with focus.

layershifter · 2017-06-02T16:02:35Z

src/modules/Dropdown/Dropdown.js

    if (keyboardKey.getCode(e) !== keyboardKey.Enter) return
    e.preventDefault()

+    if (search && _.isEmpty(this.getMenuOptions())) return


Disables Enter key when nothing find.

layershifter · 2017-06-02T16:02:52Z

src/modules/Dropdown/Dropdown.js

    // Do not access document when server side rendering
-    if (!isBrowser) return
-    document.addEventListener('mouseup', this.handleDocumentMouseUp)
+    if (isBrowser) document.addEventListener('mouseup', this.handleDocumentMouseUp)


Minor refactor.

layershifter · 2017-06-02T16:02:58Z

src/modules/Dropdown/Dropdown.js

    this.isMouseDown = false
    // Do not access document when server side rendering
-    if (!isBrowser) return
-    document.removeEventListener('mouseup', this.handleDocumentMouseUp)
+    if (isBrowser) document.removeEventListener('mouseup', this.handleDocumentMouseUp)


Minor refactor.

layershifter · 2017-06-02T16:03:32Z

src/modules/Dropdown/Dropdown.js

+
+    if (!search) return this.toggle(e)
+    if (open) return
+    if (searchQuery.length >= minCharacters || minCharacters === 1) this.open(e)


Update behaviour with click on search input.

…React into fix/dropdown-minchars

layershifter · 2017-06-06T09:17:50Z

test/specs/modules/Dropdown/Dropdown-test.js

-        .simulate('change', { target: { value: faker.hacker.noun().substring(0, 1) } })
-
-      dropdownMenuIsClosed()
-    })


Test was moved to another section.

layershifter

@levithomason We're ready for review there, will be awesome to hear your thoughts.

timworx · 2017-06-06T18:38:19Z

Would be great to see this merged.

It fixes a considerable bug in handleSearchChange. Within that function setState happens in an if block that keeps it from being called when you're deleting the last character in the searchQuery.

layershifter · 2017-06-07T07:43:01Z

src/modules/Dropdown/Dropdown.js

-
-    this.close(e)
-  }
+  toggle = e => this.state.open ? this.close(e) : this.open(e)


Method was updated because its logic was moved to handleClick

layershifter · 2017-06-07T20:32:18Z

@timworx thanks for the feedback, I missed this problem, hovewer it's fixed.

Just a note, @levithomason I think you should ship this in 0.68.6, because it fixes critical bugs, without this PR the Dropdown with a search prop is completely unusable.

layershifter · 2017-06-21T14:51:51Z

@levithomason please take an attention to this PR.

levithomason · 2017-06-22T01:06:16Z

The next shipment (here in a few minutes) will be breaking because of #1730 which has already been merged. We won't update previous versions until we hit a stable 1.0 release. Bugs and features will be shipped as fast as possible without going backwards.

I'll demo this and see about shipping it now in 0.69.0.

levithomason · 2017-06-22T01:28:08Z

Released in [email protected]

levithomason reviewed Jun 1, 2017

View reviewed changes

layershifter force-pushed the fix/dropdown-minchars branch from e2bbd17 to 3124e5c Compare June 1, 2017 14:25

layershifter force-pushed the fix/dropdown-minchars branch from 3124e5c to a3f6639 Compare June 2, 2017 10:30

layershifter changed the title ~~fix(Dropdown): fix minCharacter prop~~ fix(Dropdown): fix minCharacter prop behaviour Jun 2, 2017

wip

42c14ce

layershifter force-pushed the fix/dropdown-minchars branch from a3f6639 to 42c14ce Compare June 2, 2017 15:57

layershifter commented Jun 2, 2017

View reviewed changes

src/modules/Dropdown/Dropdown.js

noResultsMessage: 'No results found.',

openOnFocus: true,

Copy link

Member Author

layershifter Jun 2, 2017

Choose a reason for hiding this comment

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

Only prop sort there.

layershifter commented Jun 2, 2017

View reviewed changes

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

347e437

…React into fix/dropdown-minchars

layershifter commented Jun 6, 2017

View reviewed changes

layershifter added the ready for review label Jun 6, 2017

layershifter commented Jun 6, 2017

View reviewed changes

layershifter force-pushed the fix/dropdown-minchars branch from ec28a3a to fb14678 Compare June 7, 2017 07:17

test(Dropdown): add missing tests

5f70d73

layershifter force-pushed the fix/dropdown-minchars branch from fb14678 to 5f70d73 Compare June 7, 2017 07:33

layershifter commented Jun 7, 2017

View reviewed changes

layershifter mentioned this pull request Jun 8, 2017

Dropdown not clearing characters #1745

Closed

layershifter mentioned this pull request Jun 21, 2017

Dropdown: one character cannot be deleted with backspace #1784

Closed

levithomason merged commit 9287f9c into master Jun 22, 2017

levithomason deleted the fix/dropdown-minchars branch June 22, 2017 01:14

layershifter mentioned this pull request Jun 30, 2017

Dropdown selection deletion of last letter doesn't work in production, and works ok in development mode #1819

Closed

levithomason added the breaking-change label Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Dropdown): fix `minCharacter` prop behaviour #1722

fix(Dropdown): fix `minCharacter` prop behaviour #1722

layershifter commented May 31, 2017 •

edited

Loading

levithomason Jun 1, 2017

levithomason Jun 1, 2017

layershifter Jun 2, 2017

codecov-io commented Jun 1, 2017 •

edited

Loading

layershifter commented Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 2, 2017

layershifter Jun 6, 2017

layershifter left a comment

timworx commented Jun 6, 2017

layershifter Jun 7, 2017 •

edited

Loading

layershifter commented Jun 7, 2017 •

edited

Loading

layershifter commented Jun 21, 2017

levithomason commented Jun 22, 2017

levithomason commented Jun 22, 2017

fix(Dropdown): fix minCharacter prop behaviour #1722

fix(Dropdown): fix minCharacter prop behaviour #1722

Conversation

layershifter commented May 31, 2017 • edited Loading

Behaviours

TODO

onClick

minCharacters = 1

minCharacters = 3

onFocus

minCharacters = 1

minCharacters = 1

minCharacters = 3

onChange

minCharacters = 1

minCharacters = 3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 1, 2017 • edited Loading

Codecov Report

layershifter commented Jun 2, 2017

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

layershifter left a comment

Choose a reason for hiding this comment

timworx commented Jun 6, 2017

layershifter Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter commented Jun 7, 2017 • edited Loading

layershifter commented Jun 21, 2017

levithomason commented Jun 22, 2017

levithomason commented Jun 22, 2017

fix(Dropdown): fix `minCharacter` prop behaviour #1722

fix(Dropdown): fix `minCharacter` prop behaviour #1722

layershifter commented May 31, 2017 •

edited

Loading

`minCharacters` = 1

`minCharacters` = 3

`minCharacters` = 1

`minCharacters` = 1

`minCharacters` = 3

`minCharacters` = 1

`minCharacters` = 3

codecov-io commented Jun 1, 2017 •

edited

Loading

layershifter Jun 7, 2017 •

edited

Loading

layershifter commented Jun 7, 2017 •

edited

Loading