-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Dropdown: add support for allowAdditions #356
Dropdown: add support for allowAdditions #356
Conversation
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 |
Current coverage is 94.89% (diff: 100%)@@ 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
|
@levithomason I added a simple example to the docs. |
{ text: 'Spanish', value: 'Spanish' }, | ||
{ text: 'German', value: 'German' }, | ||
{ text: 'Chinese', value: 'Chinese' }, | ||
]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@levithomason Thanks for your very useful remarks! I will look into it as soon as possible. |
@levithomason I'm trying to add tests for my component, but when I try to run them using
I haven't changed anything in the config file. Do you know immediately what this problem could be? |
@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 ... :) |
Which version of Node do you have? This project requires Node 6. See CONTRIBUTING.md Getting Started for more. |
Well, that will be it. We're working with Node 4. Will try with Node 6 next time :) |
@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. |
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 " 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. |
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 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). |
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
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
Had no idea 😄, though it seems like a common enough problem and great feature we should add still. Thoughts? |
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 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 |
Sure, something like |
Alright, I will look into it probably tonight or tomorrow. |
@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
Is there a reason for these discrepancies, or are they just things that were overseen? |
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. |
Okay, I might work on these issues then after this one is ready :) |
@levithomason Sorry for the delay. I managed to incorporate the Could you take a look to see whether this looks good to you? |
Awesome! Pulling and reviewing now. |
onAddition: PropTypes.func, | ||
|
||
/** Position of the `Add: ...` option in the dropdown list ('top' or 'bottom'). */ | ||
additionPosition: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-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'],
},
}
]), | ||
|
||
/** Called with the new value added by the user. Use this to update the options list. */ | ||
onAddition: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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! |
@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? |
Will do, thanks |
this.setState({ | ||
options: newOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed since there is now no use of state for options.
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 ...) |
@levithomason What do you think of removing the If I think of my use case I definitely don't need the |
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 |
@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? |
Awesome, let's get tests passing and merge it. |
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. |
Woop! Thanks for the work here, merging. |
Released in |
No description provided.