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

Change isOptionDisabled to have the same inputs as isOptionSelected #2695

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

SimeonC
Copy link
Contributor

@SimeonC SimeonC commented Jun 8, 2018

As we pass the selected values to isOptionSelected, it makes sense to pass them to isOptionDisabled as well. Allows for much more dynamic select lists.

Some context

In the code for the following image I have auto selecting checkboxes, I'd like to also auto disable them using very similar logic which requires isOptionDisabled to be passed the list of selected options.
Code snippets after the image.

screencast 2018-06-08 20-35-45
my data structure simulates groups by concatenating ids, something like this:

items={[
  ...
  { label: 'fr 1', value: 'sh1,sh2' },
  { label: 'Shop 1', value: 'sh1' },
  { label: 'Shop 2', value: 'sh2' }
  ...
]}

Then my select implementation goes something like this:

<Select
  isOptionSelected={(option, options) => {
    for (let i = 0; i < options.length; i += 1) {
      if (
        options[i].value === option.value ||
        options[i].value.indexOf(option.value) >= 0
      )
        return true;
    }
    return false;
  }}
  options={...}
/>

As we pass the selected values to isOptionSelected, it makes sense to pass them to isOptionDisabled as well. Allows for much more dynamic select lists.
@gwyneplaine gwyneplaine self-assigned this Jun 19, 2018
@gwyneplaine
Copy link
Collaborator

@JedWatson interesting use case, with a simple seemingly non-intrusive addition to the isOptionDisabled api. Would be good to get your thoughts on this

@gwyneplaine gwyneplaine reopened this Jun 27, 2018
@gwyneplaine
Copy link
Collaborator

@SimeonC, this looks good, but in the case of isOptionDisabled, I'm not entirely sure what the value of passing through the selectedValue array, it may make more sense to pass through the options array instead. Would like your thoughts on this, otherwise this lgtm

Repository owner deleted a comment from gwyneplaine Jun 27, 2018
@JedWatson
Copy link
Owner

I'm on the fence about this.

On the up-side, as an API addition it does seem consistent.

On the down-side, it seems like this is pretty easy to do if you're controlling the value:

const { selectValue } = this.state;
<Select
  isOptionDisabled={(option) => {
    for (let i = 0; i < selectValue.length; i += 1) {
      if (
        selectValue[i].value === option.value ||
        selectValue[i].value.indexOf(option.value) >= 0
      )
        return true;
    }
    return false;
  }}
  onChange={newValue => this.setState({ selectValue: newValue })}
  options={...}
  value={selectValue}
/>

@SimeonC do you think it's reasonable to leave as-is and go with the example above? or do you feel strongly that we should include this addition to the API?

@SimeonC
Copy link
Contributor Author

SimeonC commented Jun 27, 2018

Yea, doing that makes sense and my other PR got merged which is what I'll probably be using in the case here - different thinking on different days 😜.
I don't feel strongly about it but I'll outline some thoughts of mine below:

I just think it would be good if the handlers named by the same pattern isOption*** took the same function signature for consistency, in my head having 2 handler props called isOptionDisabled and isOptionDisabled the functions passed to them should function the same. (Also I think having them different may cause confusion for some people)

I think the only real use case would be if you import the function isOptionDisabled from a shared utils file as even if you bind it you have to make sure that all the components have the same named prop/state - though this is just convenience as you could still use a wrapper function. eg:

import { isDisabled } from './utils';
<Select isOptionDisabled={isDisabled} ... />

Up to you guys though, I'm happy either way.

@JedWatson
Copy link
Owner

Thanks for the feedback @SimeonC

I'm going to merge this, I think the point about having a shared function that you can't enclose the value in is good enough to get it over the line.

@JedWatson JedWatson merged commit 4b500d9 into JedWatson:v2 Jul 23, 2018
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.

None yet

3 participants