Implemented allowCreate and more flexible add item on comma functionality

[ryanzec]

I took the pull request #660 (which relates to #658 and #586) and re-did it as this is a huge show stopper for me on using this library and that implementation seemed a bit broken (and the 0.9.1 does not have some of the functionality that I also need the 1.0.0-beta does).

This adds back in the allowCreate functionality with the tests reworked to pass (I also fixed an existing failing test as it seemed to be using the old setPropselectorChild method instead of setPropsForChild)

UPDATE:

I added another commit here to add a more flexible solution to the old add item on comma when allowCreate and mutli are enabled. My solution allows you to specific the keyCode to use to add a new item (set with the addItemOnKeyCode prop). While comma is pretty common, it is not always the best option and making it flexible like this it not that much difference in the implementation. This also allows you to turn off this feature which was not possible in the older implementation which i think is not completely uncommon. By default the addItemOnKeyCode prop is set to null so that the user need to explicitly enable this functionality.

Also, I have no idea what the @coveralls messages mean. I did run coverage reports before and after and the coverage did not go down after my src / test updates (they actually went up on 1 or 2 src files but only marginally).

If I could get an update if this will be an acceptable PR (or if any changes are needed to get this merged) and included into a npm tagged beta release relatively soon that would be great. Without these changes, using this library is a show stopper. Since 0.9.1 is also missing functionality the 1.x beta version has, using that version is also a show stopper. If these features are not going to be able to be consumable through npm relatively soon, I am going to have to publish my own version to be able to move forward with this library which I would prefer not to do.

[coveralls]

Changes Unknown when pulling 3234d96 on ryanzec:allow_create into * on JedWatson:master*.

[coveralls]

Changes Unknown when pulling 2fffd26 on ryanzec:allow_create into * on JedWatson:master*.

[ryanzec]

This also resolve the issue I put in #987

[bIgBV]

@ryanzec thanks for picking this up! I was out of town for a week and had just started work on this issue when I saw your PR. Also, the addItemOnKeyCode option is very useful.

@omgaz any issues stopping this PR from being merged and released? This is a pretty major show stopper for us to upgrade to react v15.

[ttrmw]

@ryanzec I couldn't get allowCreate et al to work with your branch. Component code is:

//..                                      
        <Select name="pipeline-filter"                                            
                     value={this.state.value}                                          
                     options={this.options}                                            
                     onChange={this.handleSelectChange}                                
                     multi={true}                                                      
                     allowCreate={true}                                                
                     addItemOnKeyCode={188}                                            
        />           
//..

But the select doesn't respond as expected to a comma in the input.

[JedWatson]

@ryanzec thanks for this PR, and apologies it's taken so long to get to. I've been flat out both at work and home, but getting this in and 1.0 released properly is at the top of my OSS "todo" list. I'll make time for it this weekend if I don't get a chance before then to do a proper review & publish an update.

In the meantime if anyone else can review this & provide feedback that would be helpful too (thanks @ttrmw for jumping on it)

[ryanzec]

@JedWatson
Sounds good.

@ttrmw
I appreciate the feedback however I am a little confused on your the first part of your comment:

I couldn't get allowCreate et al to work with your branch

I assume by et al you mean and others (sorry, not used to seeing latin phrases) which leaves me to believe that you could not get anything to work, is that correct?

Even without understanding your comment fully, I did add your example code into the examples in my branch and then loaded it up in the browser and everything seemed to work fine. As I started to type, the Add ... option became available. When I hit the comma key, the focused value in the auto complete was selected. Now if you have a different idea on how the implementation should work, that is a different story. I tried to implementation in a way that was not commonly uncommon.

I think the implementation of the Add ... option showing up seems like a pretty standard way for these types of components to work. The implementation of the comma is something I pulled from how stackoverflow works. If I start to type and then arrow key to a selection and then hit the space (which is what they use for the delimiter), that adds the value that was selected and that is basically how this implementation works with the difference being that the add option is shown here (not sure how add works there, apparently I don't have the rep to test that out). I am basically making the addItemOnKeyCode key work like the enter key which to me seem like the most intuitive thing.

Now if the allowCreate stuff is not working at all for you, then I am not sure what to say as the allowCreate related tests all pass, the example in the examples seems to work fine for me with this code:

<Select
    allowCreate={this.props.allowCreate}
    value={this.state.value}
    multi
    placeholder="Select your favourite(s)"
    options={this.state.options}
    onChange={this.handleSelectChange}
    addItemOnKeyCode={188}
/>

and I don't see any property differences that would cause the allowCreate to function any different in comparison to your example. If the implementation is working differently than you were expect, that I would need more details on what you were expecting and that is more of a decision for the maintainers of this library to decide on (though my 2 cents on implementation is obviously that way I have done it here).

[ttrmw]

Hey sorry for the confusing wording. I meant none of it is working for me. I'm onboard with the API you've chosen here but not looked at the code yet - and the behaviour you describe sounds totally reasonable, but I cannot currently test it. If I can get it working I'll be happy dig in to your implementation if that'd be helpful.

So to be more specific: there is no way for me to add a new option with the component code as specified above. I don't get the 'Add..' prompt, and the select does not respond to my chosen keycode.

I'm away from now until next week but would be happy to step through the code and see if I can identify the issue then, if that'd be helpful!

[ryanzec]

@ttrmw Are you sure that you are running against the right code as it works for me:

and then I can add the new value just fine:

I have tested this in Chrome, FireFox, Safari, and IE 10. Not sure whatelse to say. Only things I can say is for someone else to try this branch to see if it is not working for them as everything is working for me.

[omgaz]

Awesome job @ryanzec, just missing support for the simpleValue prop type. simpleValue will serialise the object to a CSV string. There's a pre-existing glitch when you start typing and there are two lines you get a spacer between the control and the options menu, but I think that's in master and should be resolved separately.

Here's an example:

The values are stored correctly as Chocolate,Test,Something Else,Vanilla,Strawberry,Net but they're not displayed.

[coveralls]

Changes Unknown when pulling 3bf011a on ryanzec:allow_create into * on JedWatson:master*.

[ryanzec]

@omgaz
Thanks for the feedback, I don’t use the simple value myself so I did not test that. The issue was that the expandValue() method was assuming that the value was stored in the this.props.options but since it is a dynamically created options, it is not there. I modified the code so that if it does not find the value in this.props.options and allowCreate is enabled, it will assume this is a new value and create the object.

As I was doing more testing, I also noticed that the removeValue() method was not working with created tags. This was happening because the code assumes the object would be exact the same object (I assume since it assumes the value would come directly from the this.props.options) and since that it not the case, I modified that code to do an equality check of the valueKey and labelKey if the object has the create property set to true.

I also refactored the code to create the new option to use the this.props.valueKey and this.props.labelKey.

I did look at the spacing issue and that is something that is pre-existing and not a regression of this feature, it is just more noticeable when this feature is enabled. The issue seems to happen when the user types of the value more instead of just selecting a value. Here is an existing example selecting 3 items and everything works fine:

Here is me selecting the same 3 items for the example except I am typing out the value before selecting them:

For whatever reason when I type the value out, the main element (.Select) seems to think it is taller than it should be as soon as some causing the component to rendering again, it fixes itself. that seems like an issue which should not prevent the PR from going through.

If anyone else find any other issues related to this PR, please let me know and I will take a look at it.

[omgaz]

🏆

[f2net]

Thank you very much @ryanzec :)
I hope this PR gets published because it's much needed.

[marshallsherman]

This looks good. @JedWatson Any idea on a timeframe for getting this merged in?

[coveralls]

Coverage increased (+0.2%) to 91.697% when pulling 2fee64c on ryanzec:allow_create into 8373ca4 on JedWatson:master.

[ryanzec]

FYI, I rebased this branch with latest changes from master to prevent conflicts.

[ryanzec]

@JedWatson So it is getting close to 2 weeks since your last response, anything else I can do to help more this PR forward?

[seanmheff]

Looking forward to this getting merged. Great work!

One thing that has crossed my mind relates to the addItemOnKeyCode prop. I wonder would the prop be better suited as an Array. I think it would be more flexible if you could pass in a bunch of key codes instead of one additional one.

Anyone have any thoughts on this?

[ryanzec]

@seanmheff I though about that myself however my personal opinion is that an input should be limited to 1 specified key that would enter tags as to me it would seems a little weird if an input would enter tags with seemingly random different keys. I can't think of a single valid use case that would require needing multiple keys to perform this functional and I always air on the side of omitting code for that hypothetical edge use case that might happen even though I can think why.

If anyone can provide a real valid use case for needing this, I would be more than happy to update the PR (though saying this as a developer seems to always bite me in the ass, it should be relatively simple to add in).

[seanmheff]

@ryanzec the use case I had in mind was tags for email addresses.

I once had to build an input that mirrored how gmail compose new message tags worked. The client wanted many different characters (comma, colon, semicolon, tab, carriage return) to cause tags to be created. This is how it works in gmail. It's quite a good user experience.

This was the use case I had in mind.

But you are probably right, it is very much an edge case and lets not get this PR get into the weeds!

[klaemo]

This is probably not my place to tell anyone what to do, but maybe it might be beneficial for this project if @JedWatson could add more collaborators? With push and publish access.

[danbovey]

@klaemo no that's fair. I think with the amount of all the other issues and PRs, it just needs another maintainer to go through and close a lot of old and useless things 😃

[JedWatson]

I've actually added six collaborators to this project already, unfortunately nobody on that list has jumped in on this particular feature. I asked another friend (whose company also uses this project in production) to join earlier this week to help me out and hopefully that'll happen, there's a bit to get up to speed with though. Triaging 80 PRs and 380 issues is a big ask for anyone in their free time, and that's after the great work @bvaughn did recently.

If anyone else wants to step in, I'd love the help - ping me on Twitter and maybe we can Skype to get up to speed?

[ryanzec]

So I just wanted to let people know that I will not longer be updating this PR as I have decided to go with my own open sourced auto complete / tagging component (so hopefully the PR is good as is right now or someone else will be willing to update it if needed). I originally started the project that required this functionality 2 months ago when my own component was not really up to par on the functionality of this however it now is (at least with the functionality I am concerned with) and I am just running into to many issue with this component (more a dependent of this component).

I have been running into a number of issues that all seem to relate with the auto sizing input. Issues range from on IE the beginning 2-4 characters get cutoff once you have entered 3-4 spaces to the tags / placeholder text shifting up to be flush with the top border (no longer having the padding) with all browsers on Windows only (Mac seems fine) to the input container becoming as wide as the longest tag regardless of the parents width, and my use case can have some pretty long tags, instead of wrapping (this happens only in Chorme on Windows) and that is not mentioning a couple of other issues that I already fixed with custom code. Not sure if the issues are directly related with the auto sizing input or if the bootstrap / custom application css we have is interfering with it somehow but there are just to many issues I have seen to feel confident in continuing to use this library. My library does not use such a component as the tags are displayed outside the input and therefore does not suffer from these issues.

Hopefully this PR will get merged in as it is sorely needed for a library like this and I still think that this library will be really good once some of the issues are resolved.

[JedWatson]

Thanks for letting us know @ryanzec, and also for all your contributions to react-select 😀

I know I've been promising to get this into shape for a final release for a while now and have been held back by work / becoming a dad / injury in the last couple of months, but as soon as I get a few hours to focus on it this is still at the top of my list and I'll take it forward from here.

[abiskop]

Thanks for providing this PR, @ryanzec. I merged it into my fork and quickly tested it, when I noticed that this line
https://github.com/JedWatson/react-select/pull/999/files#diff-04191649796841200b33375bdff7c78fR870
causes problems when 'labelKey' and/or 'valueKey' are set, i.e. label and value are custom props of the option object. Just pointing this out - I understand you are not actively maintaining this PR anymore.
@JedWatson I might be able to fix this and provide an updated PR. However I can't tell when I will get to it.

[agirton]

@JedWatson I pinged you on Twitter, but just in case I'd be happy to pick this up. It looks like the remaining features needed are the SimpleValues and the ability to use labelKey and valueKey. One other area is the VirtualizedSelect library should probably be updated to include the ability to create and show the label in the dropdown.

cc @abiskop

[bIgBV]

@JedWatson I have been following this PR and this issue in general pretty closely since the last few months and would love to help in any way possible.

[nadavspi]

I've found a bug for this branch and submitted a PR to @ryanzec's fork: ryanzec#1. I'll be happy to also submit to a different fork if someone else is taking over.

[ryanzec]

@nadavspi I looked at the code (did not test the code but the CI test part seem to pass even though coverage is now lower) and it seemed simple enough so I merged it.

[maullerz]

@JedWatson how about that PR?

[danbovey]

Is there a list of things that is left in this PR that's stopping you pressing that merge button? If not can we get a comment with a To-Do list please?

I've been following this PR a couple days after it was created and I didn't understand why it wasn't merged then and there, and I still don't understand now.

---

EDIT

The only thing I can find that is wrong with this merge, that is stopping @JedWatson from pressing the merge button rn, is the dist folder and examples/dist isn't built... and there's a couple of simple lines of conflicts.

[jdelStrother]

FWIW we've started using ryanzec's branch in production, not run into any difficulties with it.

[lopezjurip]

When using custom keys (valueKey, labelKey) this causes:

Uncaught TypeError: Cannot read property 'toLowerCase' of undefined

[nadavspi]

Yeah, it needs to be changed to:

if (String(option[this.props.labelKey).toLowerCase() === filterValue || String(option[this.props.valueKey]).toLowerCase() === filterValue) {

[jebarjonet]

@nadavspi cool ! note that you inadvertently forgot a ] 👼

if (String(option[this.props.labelKey]).toLowerCase() === filterValue || String(option[this.props.valueKey]).toLowerCase() === filterValue) {

[lvnr]

@JedWatson I know you're probably super busy, but could you please tell when can we expect this to be merged?
Thanks a lot.

[jorgenhorstink]

I hope @JedWatson can find a couple of hours checking this PR soon. Becoming a dad makes one rearranging his priorities, so I understand time is not in abundance :-)

I really could use this feature as well. It would allow me to create a React Component for selecting recipients of e-mail, and adding non existing e-mail addresses.

[t1mmen]

@JedWatson If you're able to get this merged soon, email me your shipping address and size/color preference and I'll send https://www.amazon.ca/Kusteez-Comfortable-Onesie-Bodysuit-Parody/dp/B01913LPQE your way :)

[alur222]

we also need this badly. :(

[jelenajjo]

Same here, whats problem @JedWatson ?

[t1mmen]

Apologies in advance if this is inappropriate – feel free to delete if so.

We couldn't wait for this any longer, so I had to look for alternatives. https://github.com/furqanZafar/react-selectize seems decent so far. It has an allowCreate-like feature, as well as option groups (ref #59, #191, #1004)

[bvaughn]

Not inappropriate at all, @t1mmen! Glad you found a work around and that you were able to share it with others. 😄 ❤️

[JedWatson]

So I need to explain why this hasn't just been merged.

With this rebuild, I've actually been trying to simplify react-select (e.g. I halved the amount of code) by architecting it better and making it more flexible in the right way.

The idea I've been working on for allowCreate is to implement a wrapper component (like Select.Async or the way react-virtualised is integrated) that dynamically adds an option ("create {input}") and takes a callback when it's selected. This requires minor modifications to the base Select component and the wrapper to be written.

I still haven't actually had enough solid time to actually work out the code for this, and in the meantime this is a really good PR if I hadn't been planning to implement a totally different API for it.

If I'm not going to get that other idea implemented in the next week I'm going to call that a "next major version" issue and merge this in so everybody who needs it gets unblocked, and live with having the base Select component handle more concerns than I wanted it to.

(and @t1mmen, thanks for that onesie offer, that's amazing - totally made me grin. no worries linking to react-selectize, that looks great and if people are better served by that then I'm happy, open source is not a zero sum game! react-autosuggest is also a fantastic component)

[shtefcs]

@JedWatson Tnx for the replay, I appreciate it really .

[bvaughn]

Alternate implementation (based on Select.Async approach) for consideration: #1187

It's longer but it also makes progress on a few related goals (eg pulling default behavior out of base Select and into separate modules or HOCs).

[bvaughn]

Thanks for submitting this PR @ryanzec and thanks to all of the others who commented on it and gave feedback. I took the basic idea and implemented it in a way that's more in line with @JedWatson's long term vision for react-select (see PR #1187). That PR has been merged so I'm going to close this one. Look for another RC release soon with this newly-added functionality!