Why was comma create new tag functionality removed?

[ryanzec]

So there is this block of commented out code:

// case 188: // ,
//  if (this.props.allowCreate && this.props.multi) {
//      event.preventDefault();
//      event.stopPropagation();
//      this.selectFocusedOption();
//  } else {
//      return;
//  }
// break;

and I was wondering why it is commented out? Being able to create a new tag on the comma key is a pretty big deal for me and was wondering when this would be added back in?

[keul]

See #960

[ryanzec]

@keul Not exactly sure how the newOptionCreator option relates to this?

[bvaughn]

FWIW that line was added, commented out, in c3d5e01. Not sure why, but it was commented out from the code and added to the "TODO" list (where it remains currently).

[bvaughn]

Took a quick look at this and it looks like there's a good bit more complexity than just the commented-out lines. Looks like support for newOptionCreator would need to be added along with some changes to setValue / getValueArray to supporting selecting a newly-added tag. Seems tricky.

[ryanzec]

@bvaughn It did not seem too tricky. I took the PR #660 which seemed abandon, the newOptionCreator seemed to be already working there, it was just the allowCreate that was not working. I refactored the code to get the allowCreate working and then refactored the commented out code that used to do the comma create new item stuff into a slightly more flexible solution in which you can define any keyCode to perform that functionality (I did not have to make any changes to setValue / getValueArray in order to get selecting a newly created tag to work).

I created the PR #999 with those changes and all tests pass (along with the new ones for the re-implemented functionality), just waiting on feedback on it. Chance are I am going to publish my own version on NPM this weekend so I can continue along with my project as this functionality is 100% a show stopper.

[bvaughn]

Oh cool. I was unaware of #660. I was just working from master, where the property is defined but not used or referenced anywhere.

[bvaughn]

By the way, I believe PR #1187 should resolve this issue. Please feel free top give the branch a spin and let me know if you have any concerns or other feedback. 😄

[bvaughn]

This issue no longer exists in the 1.x code (as of PR #1187) and so I'm going to close it!

Look for an updated RC with this functionality soon. 😎

[GuillaumeCisco]

Is this fix in the rc.1 code?

[bvaughn]

Yup. Comma creates a new tag by default, as does ENTER and TAB

[GuillaumeCisco]

I've just resinstalled the package from the master branch.
The only way to make it works is to add a filterOptions attribute:

    filterOptions(options, filter, currentValues) {
        let filteredOptions = options.reduce((memo, option) => {
                // Optgroup support
                if (!('value' in option)) {
                    memo.push(option);
                }

                // If a filter is present
                else if (filter && filter.length >= 1) {
                    let valueNormalized = option.value.toLowerCase().trim();
                    let filterNormalized = filter.toLowerCase().trim();

                    if (valueNormalized.indexOf(filterNormalized) > -1) {
                        memo.push(option);
                    }
                }

                // Show everything available that's not already selected if no filter is used
                else if (!this.isSelected(currentValues, option.value)) {
                    memo.push(option);
                }

                return memo;
            },
            []);

        // Only display `Add ${filter}` if no other options are available
        if (filteredOptions.length < 1 && filter) {
            filteredOptions.push({
                label: filter,
                value: filter,
                create: true
            });
        }

        return filteredOptions;
    }

But it only works with ENTER and TAb, no COMMA, but the code should handle it...
I don't really understand what's going on :/

[bvaughn]

I've just resinstalled the package from the master branch.

Why not just install the RC from NPM? npm i --save react-select@1.0.0-rc.1

The only way to make it works is to add a filterOptions attribute:
But it only works with ENTER and TAb, no COMMA, but the code should handle it...
I don't really understand what's going on :/

Have you tried just copying the Creatable demo as a starting point?

[GuillaumeCisco]

Just installed the rc.1 again for trying.
I think my mistake was to import Cratable from react-select and using it as the component...
I was using <Creatable {...props}/> instead of <Select.Creatable {...props}>, I think that's why I've never been able to get a breakpoint in the Creatable code...

Anyway, I have now a new issue :
Uncaught RangeError: Maximum call stack size exceeded defaultFilterOptions.js?0b45:1

[bvaughn]

Ah, right. Async and Creatable are exported as static properties on the
main Select component so you'd need to use Select.Creatable or import it
like import { Creatable } from 'react-select'

On Sep 8, 2016 6:26 PM, "Cisco Guillaume" notifications@github.com wrote:

Just installed the rc.1 again for trying.
I think my mistake was to import Cratable from react-select and using it
as the component...
I was using <Creatable {...props}/> instead of <Select.Creatable
{...props}>, I think that's why I've never been able to get a breakpoint
in the Creatable code...

Anyway, I have now a new issue :
Uncaught RangeError: Maximum call stack size exceeded
defaultFilterOptions.js?0b45:1

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#987 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznYW_QVdPu9LVhBsSrg6DbwMTSCLaks5qn-LggaJpZM4ImpHQ
.

[bvaughn]

No idea about the RangeError you mention. Does not sound related to my changes. File a separate issue or ask on Stack Overflow?

[GuillaumeCisco]

Thanks for the quick response @bvaughn !
Looks like there is an infinite loop in the code... Don't know why.
I'm investigating, and I will open an issue if I find why.

[GuillaumeCisco]

Ok, just found why we can have an infinite loop here.
It's due to this line of code :

// TRICKY Compare to all options (not just filtered options) in case option has already been selected).
// For multi-selects, this would remove it from the filtered list.
var _isOptionUnique2 = this.isOptionUnique({ option: option, options: options });

If options is undefined, we get an infinite loop.
So I replace my <Creatable {...props} options={options}/> by <Creatable {...props} options={options || []}/>
But I think it sould be handled by react-select, so I think I'm going to create an issue ;)

[bvaughn]

Hmm. IMO options should be required to be an Array. An empty array is fine
but undefined/null should be handled outside of react-select

On Sep 8, 2016 7:48 PM, "Cisco Guillaume" notifications@github.com wrote:

Ok, just found why we can have an infinite loop here.
It's due to this line of code :

// TRICKY Compare to all options (not just filtered options) in case option has already been selected).
// For multi-selects, this would remove it from the filtered list.
var _isOptionUnique2 = this.isOptionUnique({ option: option, options: options });

If options is undefined, we get an infinite loop.
So I replace my <Creatable {...props} options={options}/> by <Creatable
{...props} options={options || []}/>
But I think it sould be handled by react-select, so I think I'm going to
create an issue ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#987 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznZZGGbOxtOs88GI_Dc1QkRMHTUJ_ks5qn_YIgaJpZM4ImpHQ
.

[GuillaumeCisco]

Hum, any way to declare it in the PropTypes?
I was thinking about isRequired reading this thread facebook/react#2166
But it forces users to declare the options attribute even if they want a empty array by default.
Should we not handle this case by default?

[bvaughn]

But it forces users to declare the options attribute even if they want a empty array by default.
Should we not handle this case by default?

I think this is preferable as an empty options array (in my opinion) most likely indicates an error or a misunderstanding of how the component works. But that's just my opinion. 😄

Hum, any way to declare it in the PropTypes?

Yes it is definitely possible. Jed didn't mark any propTypes as required though- a convention that was around before I started contributing (and so I'm not clear of the reasoning). As such I don't feel comfortable updating those definitions.

Looking at Select this morning I see there is some precedent for setting a default empty options array if none is provided...

var options = this.props.options || []

So I guess it wouldn't be unreasonable to handle this within Creatable even though it does violate the signature/spirit of the filter function IMO. Want to file an issue for it? 😄

[bvaughn]

If options is undefined, we get an infinite loop.

Now that the other PR I was working on is done, I'm looking at this again. I'd like to understand a little more about how or why options would be undefined. A user-supplied filterOptions function that returns undefined? Or something else?

[GuillaumeCisco]

Hey @bvaughn, in my case I was passing the options attribute like options={options} where the options list is filled asynchronously via API call with redux-sagas. By default, options was undefined. Should we raise an error when undefined is passed to the options attribute?

[bvaughn]

Hey @GuillaumeCisco,

Thanks for the quick response!

Passing options={undefined} is not sufficient to reproduce the issue you reported (at least in master).

[bvaughn]

FWIW I believe that's b'c we're now concatenating existing options before calling this.isOptionUnique.

[bvaughn]

FYI I just pushed PR #1213 to add a little better guard around this.

[GuillaumeCisco]

Thank you very much 👍