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

removeSelected prop (round 2), for optionally keeping selected values in dropdown #1891

Merged
merged 17 commits into from
Oct 25, 2017
Merged

removeSelected prop (round 2), for optionally keeping selected values in dropdown #1891

merged 17 commits into from
Oct 25, 2017

Conversation

banderson
Copy link
Contributor

I forked what @dherran did with #882 to resolve conflicts with master and to implement PR feedback from @agirton, so defer to #882 and the original issue for more details: #803

This is in a holding state until we decide what to do here. But this should work as expected, and all tests are passing.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 93.91% when pulling 6558b1e on banderson:remove-selected into 6c0ee59 on JedWatson:master.

@agirton
Copy link
Collaborator

agirton commented Jul 26, 2017

Hi @banderson thank you for this. Since this adds a new prop this will need unit tests and documentation. Can you add these?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 93.91% when pulling 3d88a77 on banderson:remove-selected into 6c0ee59 on JedWatson:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 94.017% when pulling a6992e8 on banderson:remove-selected into 6c0ee59 on JedWatson:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 94.017% when pulling a6992e8 on banderson:remove-selected into 6c0ee59 on JedWatson:master.

@banderson
Copy link
Contributor Author

banderson commented Jul 30, 2017

@agirton I just updated this to include:

  • Documentation in README to describe this feature: 3d88a77
  • Test coverage for selecting (878b880) and deselecting (3779f38) values

The original author (@dherran) already added docs to the example site.

I tried to follow the testing style that was there, let me know if there's anything else I can do to help get this in!

/cc @TrevorBurnham

README.md Outdated
@@ -104,7 +104,7 @@ The built-in Options renderer also support custom classNames, just add a `classN

You can enable multi-value selection by setting `multi={true}`. In this mode:

* Selected options will be removed from the dropdown menu
* Selected options will be removed from the dropdown menu by default. If you want them to remain in the list, set `removeSelected={true}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banderson just a minor nit: should this be "If you want them to remain in the list, set removeSelected={false}"?

@ansgarm
Copy link

ansgarm commented Sep 25, 2017

Is there anything currently blocking this (besides the true -> false change in the docs)?

We could really need this aswell and I'd be happy to help with any remaining steps so this PR can be merged :)

@ansgarm
Copy link

ansgarm commented Sep 25, 2017

Two test cases were broken btw, fixed them here:
elbstack@4c744b5

louisbl pushed a commit to louisbl/react-select that referenced this pull request Oct 10, 2017
Copy link
Collaborator

@gwyneplaine gwyneplaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to go @JedWatson

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.901% when pulling 9096af5 on banderson:remove-selected into b6fd3e7 on JedWatson:master.

@JedWatson
Copy link
Owner

Thanks everyone! This is good to land 👍

@JedWatson JedWatson merged commit 381ebc1 into JedWatson:master Oct 25, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 92.857% when pulling 41378ff on banderson:remove-selected into 5a273e1 on JedWatson:master.

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.

7 participants