Material-ui: [SelectField] Add props to disable IconButton's touchRipple

[solkaz]

Description

I would like to be able to set the disableTouchRipple prop on the underlying <IconButton /> element in a <SelectField />, but this is currently not possible, so even if the <SelectField /> is disabled, clicking the icon will trigger a ripple effect.

Images & references

Versions

• Material-UI: 0.17.0
• React: 15.4.2
• Browser: Chrome 56
• OS: MacOS

[elazzabi]

I'll take this

[oliviertassinari]

I think that it should be DropDownMenu's responsibility to apply the disableTouchRipple property when disabled is true.

@elazzabi What do you think about updating the PR to follow that logic?

[solkaz]

Should there be any consideration for cases where a user might want to disable the touchRipple but not the SelectField, and vice versa?

[oliviertassinari]

@solkaz That's something we should consider too. Would you say it's a common use-case?
If it's, we could expose the disableTouchRipple property.

[solkaz]

I do not require that feature for my particular use-case, but I had exposed that property anyway in my fork.

[solkaz]

One thought I also had was to set disableTouchRipple={disableTouchRipple || this.props.disabled} on the <IconButton /> in <Drop down menu />, so that the ripple is disabled if you explicitly specify it or if the drop down menu is disabled. That would mean you cannot have a disabled <Drop down menu /> with the ripple effect enabled.

[elazzabi]

Yeah, at first I wanted to apply it automatically if the SelectFiled is disabled.

Is there any use case when we want to disable the effect explicitly? Ex: SelectField is not disabled, but we don't want the ripple effect on it. A valid use case?

I'll update the PR once we find the right implementation :+1:

[oliviertassinari]

That would mean you cannot have a disabled with the ripple effect enabled.

@solkaz You are correct with the proposed implementation. I think that we could avoid it with the following logic:

const disableTouchRipple = typeof disableTouchRipple === 'undefined'
  ? disable
  : disableTouchRipple

[oliviertassinari]

@elazzabi Thanks for getting involved in the project.

[elazzabi]

@oliviertassinari love to be able to help :smile:

So the idea is to disable it automatically when the Dropdown is disabled? We'll go with that implementation?

[oliviertassinari]

@elazzabi Sounds good thanks. Please add unit tests and don't forget that the master branch is in a dying mode, we try to put as much effort as possible in the next branch.

[elazzabi]

Ok, so I'll correct it, add unit tests and push in next. That's it?

One point please, never done a PR correction before, should I push the new changes to my branche and make a PR, or force pushand make a PR? How does it work? Thanks 😀

[oliviertassinari]

@elazzabi The next branch is widly different from the master branch. That won't make sense their.
If you try to address an issue you are not experiencing, avoid working on that. I'm removing the good first issue tag.
@solkaz seems to have made a fork.

How does it work?

It's better to force push the changes.

[solkaz]

My team is currently using the master branch for our project, so I'd like the changes to be in there. Could I just make a PR for the master and the next branch each?

[elazzabi]

Okey, thanks ✌

[oliviertassinari]

My point was that we don't need that change on the next branch. Still, the master definitely needs it 👍

[oliviertassinari]

Regarding the good first issue tags, I have moved them to the next branch as we try transitioning.