Skip to content
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

Inconsistent behaviour on remove last item (vs remove all items) #4247

Closed
joe-oli opened this issue Oct 12, 2020 · 9 comments · Fixed by #4316
Closed

Inconsistent behaviour on remove last item (vs remove all items) #4247

joe-oli opened this issue Oct 12, 2020 · 9 comments · Fixed by #4316

Comments

@joe-oli
Copy link

joe-oli commented Oct 12, 2020

Are you asking a question?

No.

Are you reporting a bug or runtime error?

An error in your example, a runtime error, and a bug.

An error in your example in this page:
https://react-select.com/creatable

Specifically Multi-select text input and the method handleKeyDown;

  handleKeyDown = (event: SyntheticKeyboardEvent<HTMLElement>) => {
    const { inputValue, value } = this.state;
    if (!inputValue) return;
    switch (event.key) {
      case 'Enter':
      case 'Tab':
		//... etc... 
        this.setState({
          inputValue: '',
          value: [...value, createOption(inputValue)],
        });
        event.preventDefault();
    }
  };

The line
value: [...value, createOption(inputValue)],
assumes ...value is always an array; this is NOT true;
value becomes null when you remove the last item, by clicking on the individual item;

Herein lies the bug: Inconsistent behaviour when removing the last item.
When removing an individual last item (click on small-x), value = null;
When removing all items (click on big-x), value = [];

Are you making a feature request?

No. it is a bug fix.
Please make the behavior consistent on item removal, so that even your own example works.

@joe-oli joe-oli changed the title remove item Inconsistent behaviour on remove last item (vs remove all items) Oct 12, 2020
@Rall3n
Copy link
Collaborator

Rall3n commented Oct 12, 2020

@dakur
Copy link

dakur commented Oct 21, 2020

Related #4252

@ebonow ebonow added issue/bug-confirmed Issues about a bug that has been confirmed by a maintainer issue/reviewed Issue has recently been reviewed (mid-2020) and removed issue/needs-review labels Dec 5, 2020
@ebonow
Copy link
Collaborator

ebonow commented Dec 13, 2020

Documentation Issue is a duplicate of #4190

@ebonow ebonow linked a pull request Jan 14, 2021 that will close this issue
@ebonow ebonow removed issue/bug-confirmed Issues about a bug that has been confirmed by a maintainer issue/reviewed Issue has recently been reviewed (mid-2020) labels Jan 14, 2021
@ebonow
Copy link
Collaborator

ebonow commented Jan 14, 2021

Closing this as the example and behavior have been fixed.

Related: #4373

@ebonow ebonow closed this as completed Jan 14, 2021
@joe-oli
Copy link
Author

joe-oli commented Jan 14, 2021

So what is the simple answer? is removing all-items and removing last-item now setting the value to

  1. null, or
  2. [] ?

All the related issues have got me so confused.

@dakur
Copy link

dakur commented Jan 14, 2021

@joe-oli I don't know in which issue I have read it, but the answer – as I understood it – is null for now, but may be rethought to [] in future.

@dakur
Copy link

dakur commented Jan 14, 2021

@joe-oli I don't know in which issue I have read it, but the answer – as I understood it – is null for now, but may be rethought to [] in future.

Ha, it's here: #4293 (comment)

  • Clearing the value from isMulit Select will return the value null to be consistent with current implementation.
    • It should be noted that it was discussed that this approach for returning null for multi Select can be confusing and will likely be reverted back to [] in v4.0

@ebonow
Copy link
Collaborator

ebonow commented Jan 14, 2021

So what is the simple answer? is removing all-items and removing last-item now setting the value to

  1. null, or
  2. [] ?

All the related issues have got me so confused.

@joe-oli,

null per the spec for v3 as described under Normalized Values
https://github.com/JedWatson/react-select/releases/tag/react-select%403.0.3

In a recent call with Jed, Nathan, and Thomas, the argument was made that null seems a bit confusing and while rolling the spec change back only one major release later seems to admit we got it wrong, ultimately the goal is to get it right.

Therefore in v3.2, the fix was made for consistency to return null when removing the last value to avoid breaking changes,

In v4, this will likely be reversed such that a multi with no value returns empty array instead.

Hope this is clears things up. Did you have any other questions or concerns regarding this approach?

@joe-oli
Copy link
Author

joe-oli commented Jan 15, 2021

Did you have any other questions or concerns regarding this approach

Thank you, no more questions now that it is clear what to expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants