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 toggle (for multi). Includes example #882

Closed
wants to merge 6 commits into from

Conversation

dherran
Copy link

@dherran dherran commented Apr 10, 2016

Added toggle to keep/remove selected options from a multi dropdown. Set to true by default to maintain the current behaviour.

If the option is set to false, clicking an option again will simply remove the element from the array.

Fully tested locally. Works like a charm.

gif

@main-kun
Copy link

This is a very needed feature. I've implemented select with categories using optionRenderer. But categories don't make sense when items disappear from them.

@prateekjahead
Copy link

prateekjahead commented Jun 22, 2016

Any updates on this? :-/

@joegesualdo
Copy link

Any updates on this?

Bump

@dherran
Copy link
Author

dherran commented Jul 19, 2016

Are tests still outdated? if they were fixed, how can I run them again so we can sort out the build? I think the reason this pr has been ignored is because it isn't passing the tests, although it just complains of a failed npm install.

@mpodlasin
Copy link

Hi. I need this feature at my job. I used your implementation, but when there is many options (menu is scrollable), chosen option always moves to top position. Menu probably should not move at all on click.

@reintroducing
Copy link

It seems like a lot of the code has changed since this was done. Any way to get it updated for the new structure in the upstream repo and get this merged in @dherran? I'd really love to see this feature implemented.

@mpodlasin
Copy link

mpodlasin commented Aug 30, 2016

also focus is not set at chosen option (as it probably should be)

Also unit tests are a mess, how people do CI here? will it be merged even though tests are failing?
Code becomes more and more messy because of all conditionals. Without good testing suite, developing without introducing bugs will quickly become almost impossible.

I could take @dherran 's commit, merge them to current master and fix any further problems but is there a chance it will get merged in reasonable time?

@Gal-Brenner
Copy link

Any news about this?

@dherran
Copy link
Author

dherran commented Feb 10, 2017

I have merged the upstream master into my branch. All changes and samples are there now in sync with the master repo. However, it seems like the master branch build is failing, so in consequence, this pull request also fails.

If you need the feature, just pull from my fork until the branch is merged.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c12e658 on dherran:remove-selected into ** on JedWatson:master**.

Copy link
Collaborator

@agirton agirton left a comment

Choose a reason for hiding this comment

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

Overall this looks promising! Thank you! However in order to merge this will need new unit tests to cover the new feature and documentation.

@@ -18,7 +18,7 @@ function menuRenderer ({
let Option = optionComponent;

return options.map((option, i) => {
let isSelected = valueArray && valueArray.indexOf(option) > -1;
let isSelected = valueArray && valueArray.findIndex(x => x[valueKey] == option[valueKey]) > -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of including a polyfill what about just using the some method from Array?

@@ -617,7 +642,12 @@ const Select = React.createClass({
inputValue: '',
focusedIndex: null
}, () => {
this.addValue(value);
var valueArray = this.getValueArray(this.props.value);
if (valueArray.find(i => i[this.props.valueKey] === value[this.props.valueKey])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also update this to use the some method?

@@ -21,6 +21,7 @@ var MultiSelectField = React.createClass({
},
getInitialState () {
return {
remove: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use same removeSelected name for state key.

@banderson
Copy link
Contributor

banderson commented Jul 26, 2017

@dherran don't mean to step on your toes, but I also needed this feature. I rebased your changes here over master to eliminate the conflicts and addressed the issues that @agirton mentioned above. I opened it up as a new PR here to get the conversation going but open to working out the merge however you want.

Let me know what you think!

#1891

@dherran
Copy link
Author

dherran commented Jul 26, 2017

@banderson looks good to me. No objection whatsoever. We can work off your PR and close this one whenever.

@gwyneplaine
Copy link
Collaborator

As mentioned above, PR closed in favor of #1891

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.