Dropdown: add support for allowAdditions #356

dylankiss · 2016-08-01T17:27:59Z

No description provided.

levithomason · 2016-08-02T04:09:42Z

Thanks, I will review this tomorrow. Could you please add an example to the doc site so we can try it out? You can reference the other Dropdown examples in docs/app/Examples/modules/Dropdown. Just add an example file (probably types/AllowAdditions.js), then update the index.js in that directory.

codecov-io · 2016-08-02T05:31:33Z

Current coverage is 94.89% (diff: 100%)

Merging #356 into master will increase coverage by 0.03%

@@             master       #356   diff @@
==========================================
  Files            85         85          
  Lines          1110       1117     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1053       1060     +7   
  Misses           57         57          
  Partials          0          0

Powered by Codecov. Last update 4557289...ec24ec7

dylankiss · 2016-08-02T05:35:12Z

@levithomason I added a simple example to the docs.

levithomason · 2016-08-03T16:10:14Z

docs/app/Examples/modules/Dropdown/Types/AllowAdditions.js

+            { text: 'Spanish', value: 'Spanish' },
+            { text: 'German', value: 'German' },
+            { text: 'Chinese', value: 'Chinese' },
+          ]}


Let's pull these options into a const above the example. This way we can reuse them. Also, they'll be created once instead of every render.

Then, let's add a single and multiple dropdown that use the same options and both allow additions. Similar to http://semantic-ui.com/modules/dropdown.html#tagging-and-user-additions.

This raised the first bug, multiple dropdowns do not allow adding multiple additions.

dylankiss · 2016-08-03T21:40:28Z

@levithomason Thanks for your very useful remarks! I will look into it as soon as possible.

dylankiss · 2016-08-05T10:48:10Z

@levithomason I'm trying to add tests for my component, but when I try to run them using npm test, I get the following error:

05 08 2016 12:44:04.986:ERROR [config]: Invalid config file!
  SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.parseConfig (/Users/dkiss/projects/stardust/node_modules/karma/lib/config.js:284:22)
    at new Server (/Users/dkiss/projects/stardust/node_modules/karma/lib/server.js:57:20)
    at Object.exports.run (/Users/dkiss/projects/stardust/node_modules/karma/lib/cli.js:243:7)
    at requireCliAndRun (/Users/dkiss/projects/stardust/node_modules/karma-cli/bin/karma:44:16)
    at /Users/dkiss/projects/stardust/node_modules/karma-cli/bin/karma:54:12
    at /Users/dkiss/projects/stardust/node_modules/resolve/lib/async.js:44:21
    at ondir (/Users/dkiss/projects/stardust/node_modules/resolve/lib/async.js:187:31)
    at /Users/dkiss/projects/stardust/node_modules/resolve/lib/async.js:153:39
    at onex (/Users/dkiss/projects/stardust/node_modules/resolve/lib/async.js:93:22)

I haven't changed anything in the config file. Do you know immediately what this problem could be?

dylankiss · 2016-08-05T12:29:29Z

@levithomason I have changed the behavior to your suggestions and added an example for single and multi-select. I also added a few test cases to cover the code changes.

Still unable to run the tests locally, so it was just: write a test -> push ... :)

levithomason · 2016-08-05T14:49:05Z

Which version of Node do you have? This project requires Node 6. See CONTRIBUTING.md Getting Started for more.

dylankiss · 2016-08-05T15:02:03Z

Well, that will be it. We're working with Node 4. Will try with Node 6 next time :)

dylankiss · 2016-08-06T12:05:18Z

@levithomason While adding this feature I noticed that there is this behavior in the Dropdown that automatically selects the first suggestion in the list if you press enter. This makes that you can't add a substring of an existing option as a new option. I don't know what the best behavior would be to allow this, because you will still want to select your first option by pressing enter also when you are allowed to add a new one.

levithomason · 2016-08-06T21:21:48Z

This makes that you can't add a substring of an existing option as a new option.

Hm, some refactors are probably necessary. On mobile but I'll try to guide you.

There is a single "selected" item in a selection Dropdown. This is the highlighted item in the menu. It is made "active" on pressing Enter. The active items make up the Dropdown value.

There is a "selectedIndex" state key. The menu item at the index matching the selectedIndex is "selected", meaning it is highlighted. The up/down arrow keys decrement and increment the selectedIndex. When the search query changes, the selectedIndex is set to 0 (the first search result). When the search query doesn't match anything, the selectedIndex is set to -1 (iirc) so no item is selected.

Here's where the refactor may be needed. We currently match on string "includes". Perhaps when there is not an exact match, we include an item at the bottom of the list that says "Add: <search query >". If the user chooses this item, we call the "onAddition" callback.

When there are no matching results, this would replace the "No results" message that is currently shown.

Hope this helps. I'll be out the rest of today but will look more into this as soon as I can.

dylankiss · 2016-08-06T23:13:33Z

I was also considering something like that. I'd rather put it at the top of the list instead of at the bottom though. Because you could possibly have a lot of options that start with the same letter(s) and that would make the Add: ... option only visible when you scroll to the bottom. When it's at the top it's immediately clear you can add that value. The first predefined option is just one "arrow down" away then :)

What's your opinion on this?

BTW: The vanilla Semantic UI dropdown has the same behavior for user additions (also unable to add a substring).

levithomason · 2016-08-07T23:07:40Z

What's your opinion on this?

My intuition is that the most common use case will be selecting existing items and the least common use case will be adding items. Because of this, I think the more common use case should have fewer keystrokes and more visibility. Meaning, the Add: ... would be at the bottom.

Because you could possibly have a lot of options that start with the same letter(s) and that would make the Add: ... option only visible when you scroll to the bottom.

This is a really valid point. I think when adding an item you'll have a search query that will match little to no existing items. In this case you'll be able to see the Add: ... as either the first item, or just a minimal number of items down the list. Being this should be the exception, not the norm, I think it makes sense to offer the existing items first to the user. Then, in those less common edge cases that they are searching for a non-existent item with many substring matches, they may have to scroll to find the Add: ... I think that will be the intuitive behavior of a person looking for their match in the list when they can't see it.

BTW: The vanilla Semantic UI dropdown has the same behavior for user additions (also unable to add a substring).

Had no idea 😄, though it seems like a common enough problem and great feature we should add still. Thoughts?

dylankiss · 2016-08-08T06:53:45Z

My intuition is that the most common use case will be selecting existing items and the least common use case will be adding items.

I guess this depends on the specific use case for this control. For my use case it is the other way round. I basically want to use it as a text field with a history list of used values that the user could re-use, but primarily he should be able to simply add a new value here.

Ideally just tabbing from the control after having entered his custom value should call onAddition and onChange, as does the vanilla SUI Dropdown too. But that's another issue, I think.

What are your thoughts on adding another prop to select whether you want to go 'insert-first' or 'select-first' and depending on that, show the Add: ... option at the top or bottom of the list?

levithomason · 2016-08-08T08:37:22Z

What are your thoughts on adding another prop to select whether you want to go 'insert-first' or 'select-first' and depending on that, show the Add: ... option at the top or bottom of the list?

Sure, something like additionPosition={'top'|'bottom'} or similar would be great.

dylankiss · 2016-08-08T13:26:42Z

Alright, I will look into it probably tonight or tomorrow.

dylankiss · 2016-08-08T13:58:33Z

@levithomason Another discrepancy between Stardust and Semantic UI Dropdown is the way it handles blurs (and tab).

In Semantic UI, when you use the keyboard to browse through dropdown list, the placeholder changes to the entry you are currently highlighting. When losing focus after that (by clicking somewhere else or pressing tab) the highlighted value gets selected.

In Stardust

it does not update the placeholder value when browsing trough the list with the up and down keys
when clicking outside the component the value stays what it was before, or the search text stays in the search box when there was text in there
when pressing tab, the dropdown menu does not collapse and for the rest it has the same behavior as clicking outside the component

Is there a reason for these discrepancies, or are they just things that were overseen?

levithomason · 2016-08-08T18:05:46Z

There is an issue with closing on blur. There are todos in the code and tests explaining this.

All the other features can be implemented afaik, PRs welcome! It might make sense to open a separate issue/PR since the allowAdditions is entirely separate from these features.

dylankiss · 2016-08-08T21:21:48Z

Okay, I might work on these issues then after this one is ready :)

dylankiss · 2016-08-15T21:45:00Z

@levithomason Sorry for the delay. I managed to incorporate the Add: ... option (that now allows substrings) with an optional additionPosition, being 'top' or 'bottom' (default). The single select example uses no additionPosition (defaults to bottom) and the multi select uses 'top'. I also added tests to cover all the changes.

Could you take a look to see whether this looks good to you?
Thanks :)

levithomason · 2016-08-15T23:12:55Z

Awesome! Pulling and reviewing now.

levithomason · 2016-08-15T23:45:33Z

Looking much better, thanks for docs as well. There is one issue remaining. When typing a query that doesn't exists, then removing it, then "Add: " text is still shown:

This is also true for multiple dropdowns, but only when it already has a value:

levithomason · 2016-08-15T23:48:47Z

src/modules/Dropdown/Dropdown.js

+    onAddition: PropTypes.func,
+
+    /** Position of the `Add: ...` option in the dropdown list ('top' or 'bottom'). */
+    additionPosition: PropTypes.string,


-PropTypes.string, +PropTypes.oneOf(_meta.props.additionPosition),

At the top of the file:

const _meta = { name: 'Dropdown', type: META.TYPES.MODULE, props: { pointing: ['bottom left', 'bottom right'], + additionPosition: ['top', 'bottom'], }, }

levithomason · 2016-08-16T01:50:25Z

src/modules/Dropdown/Dropdown.js

+    ]),
+
+    /** Called with the new value added by the user. Use this to update the options list. */
+    onAddition: PropTypes.func,


Looking back on this now, this prop probably makes more sense as onAddItem. Mind updating?

Side note, I plan on updating the options prop to items so it follows SUI closer. Then the, Select component will use options.

levithomason · 2016-08-16T01:51:59Z

Could you take a look to see whether this looks good to you?

All comments in! Let me know if you'd like any help on this PR. I don't want to over burden you with changes. If you would like some help, just add me as a collaborator on your fork and I can make updates along with you.

Thanks much, cheers!

dylankiss · 2016-08-16T10:45:49Z

@levithomason Valid point concerning the state ;) I changed it. Also renamed the onAddition prop as per your proposal. I agree it is better.

Could you have another look please?

levithomason · 2016-08-16T19:33:14Z

Will do, thanks

levithomason · 2016-08-16T20:35:45Z

src/modules/Dropdown/Dropdown.js

    this.setState({
+      options: newOptions,


This can be removed since there is now no use of state for options.

dylankiss · 2016-08-17T07:06:07Z

Changed to all your comments. Stupid that I didn't see these :)

(Sorry for the delay in response btw, but there's a huge time difference ...)

dylankiss · 2016-08-18T07:01:23Z

@levithomason What do you think of removing the Add: prefix for our custom option and show it as a regular option? Or make it configurable?

If I think of my use case I definitely don't need the Add: prefix, but maybe other use cases require it to be clear that you are adding something.

levithomason · 2016-08-18T15:18:31Z

Don't fret over the changes and responsiveness, it's just how software goes. I appreciate all your time and effort here 👍

A configurable addition label would be superb. Maybe additionLabel and default it to Add:? What do you think?

dylankiss · 2016-08-18T16:02:35Z

@levithomason Sounds good to me. I added the prop and tests for it. This feature is now fully usable for me. What do you think?

levithomason · 2016-08-18T16:05:51Z

Awesome, let's get tests passing and merge it.

dylankiss · 2016-08-18T16:07:22Z

I just rebased before pushing without re-running the tests :) Since you added defaultProps and I did too (at another position), mine overruled yours. Should be fixed now.

levithomason · 2016-08-18T16:19:21Z

Woop! Thanks for the work here, merging.

levithomason · 2016-08-18T16:21:07Z

Released in v0.34.4

dylankiss mentioned this pull request Aug 1, 2016

Support allowAdditions option for Dropdowns #276

Closed

levithomason added the enhancement label Aug 2, 2016

levithomason mentioned this pull request Aug 3, 2016

How to access input value while typing on Dropdown #355

Closed

levithomason reviewed Aug 3, 2016
View reviewed changes

levithomason reviewed Aug 15, 2016
View reviewed changes

levithomason reviewed Aug 16, 2016
View reviewed changes

feat(Dropdown): Added support for allowAdditions

ec24ec7

levithomason merged commit ba1c9b3 into Semantic-Org:master Aug 18, 2016

levithomason changed the title ~~Added support for allowAdditions to Dropdown~~ Dropdown: add support for allowAdditions Aug 18, 2016

Dropdown: add support for allowAdditions #356

Dropdown: add support for allowAdditions #356

Conversation

dylankiss commented Aug 1, 2016

levithomason commented Aug 2, 2016

codecov-io commented Aug 2, 2016 • edited Loading

Current coverage is 94.89% (diff: 100%)

dylankiss commented Aug 2, 2016

levithomason Aug 3, 2016

Choose a reason for hiding this comment

dylankiss commented Aug 3, 2016

dylankiss commented Aug 5, 2016 • edited Loading

dylankiss commented Aug 5, 2016

levithomason commented Aug 5, 2016

dylankiss commented Aug 5, 2016

dylankiss commented Aug 6, 2016

levithomason commented Aug 6, 2016 • edited Loading

dylankiss commented Aug 6, 2016 • edited Loading

levithomason commented Aug 7, 2016

dylankiss commented Aug 8, 2016

levithomason commented Aug 8, 2016

dylankiss commented Aug 8, 2016

dylankiss commented Aug 8, 2016

levithomason commented Aug 8, 2016

dylankiss commented Aug 8, 2016

dylankiss commented Aug 15, 2016 • edited Loading

levithomason commented Aug 15, 2016

levithomason commented Aug 15, 2016

levithomason Aug 15, 2016

Choose a reason for hiding this comment

levithomason Aug 16, 2016

Choose a reason for hiding this comment

levithomason commented Aug 16, 2016

dylankiss commented Aug 16, 2016

levithomason commented Aug 16, 2016

levithomason Aug 16, 2016

Choose a reason for hiding this comment

dylankiss commented Aug 17, 2016 • edited Loading

dylankiss commented Aug 18, 2016

levithomason commented Aug 18, 2016

dylankiss commented Aug 18, 2016

levithomason commented Aug 18, 2016

dylankiss commented Aug 18, 2016

levithomason commented Aug 18, 2016

levithomason commented Aug 18, 2016

codecov-io commented Aug 2, 2016 •

edited

Loading

dylankiss commented Aug 5, 2016 •

edited

Loading

levithomason commented Aug 6, 2016 •

edited

Loading

dylankiss commented Aug 6, 2016 •

edited

Loading

dylankiss commented Aug 15, 2016 •

edited

Loading

dylankiss commented Aug 17, 2016 •

edited

Loading