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

fix: fixes dropdown multiple selection wrong behaviour for input value #115

Merged

Conversation

gabrielmicko
Copy link
Contributor

@gabrielmicko gabrielmicko commented Oct 21, 2019

PR checklist

  • I have followed the "Committing and publishing" guidelines;
  • I have added unit tests to cover my changes;
  • I updated the documentation and examples accordingly;

Changes description

Affected packages

Applying value in the dropdown was clearly a bad decision when multiselect is enabled, because it doesn't make sense. In case useFilter is applied it just messes up the list. I am sure in most cases there is a helper component that displays the values anyways.

Behaviour before:
DropdownProblematic

& after:
DropdownProblematicSolution

  • @quid/react-dropdown

@FezVrasta
Copy link
Contributor

because it doesn't make sense

May you elaborate more? Also, the example isn't very clear, I don't understand what changes.

@gabrielmicko
Copy link
Contributor Author

because it doesn't make sense

May you elaborate more? Also, the example isn't very clear, I don't understand what changes.

When you select an item the input value should not change because that is not the intended behaviour. Input value is for filtering. Selecting an item should not cause filtering at least not when multiselect is on.

@FezVrasta
Copy link
Contributor

Shouldn't this be left to the user? With your change you are effectively limiting some component functionalities, that can't be easily re-implemented in user space.

With the current implementation, instead, one can easily opt-out from this behavior overriding the function that updates the input-text value.

@gabrielmicko
Copy link
Contributor Author

From my perspective this is not a feature / functionality. This is a bad user experience. I know that this would be QA's first bug ticket about. I can't imagine a case where you want this to happen.

@FezVrasta
Copy link
Contributor

FezVrasta commented Oct 21, 2019

I'm not saying it's a ideal UX, but, for example, how can a user decide to serialize the selection into a comma separated string and show that in the input field without this feature?

In other words, this is a general purpose dropdown, we should aim to have a component that provides a basic set of functionalities that can be easily extended to suit most of the use cases. We should avoid to change it to work for a specific case.

@gabrielmicko
Copy link
Contributor Author

I'm not saying it's a ideal UX, but, for example, how can a user decide to serialize the selection into a comma separated string and show that in the input field without this feature?

I don't really understand this usecase. onSelect you could do that, no?

@FezVrasta
Copy link
Contributor

Ok I see what you mean now, let's merge it then. Thanks for the clarification.

@gabrielmicko gabrielmicko merged commit ce1edb1 into master Oct 23, 2019
@gabrielmicko gabrielmicko deleted the fix/ONE-45-Dropdown-multiple-selection-behaviour-change branch October 23, 2019 15:30
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 this pull request may close these issues.

2 participants