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

[Chip] Control clickable property #13056

Merged
merged 2 commits into from
Oct 23, 2018

Conversation

vilvaathibanpb
Copy link
Contributor

@vilvaathibanpb vilvaathibanpb commented Oct 1, 2018

Closes #13052

@vilvaathibanpb
Copy link
Contributor Author

@oliviertassinari @mbrookes Can you please review my PR?

eps1lon
eps1lon previously requested changes Oct 1, 2018
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

The visual state will now indicate that the chip is not clickable but clicking the chip will still trigger the attached handlers. This might be confusing for users.

What is the use-case for this?

You could already solve this by using:

const { clickable, onClick } = props;
<Chip clickable={clickable} onClick={clickable && onClick} />

If anything I think we should warn if users set clickable to false and onClick to a function.

packages/material-ui/src/Chip/Chip.js Show resolved Hide resolved
@eps1lon eps1lon added the component: chip This is the name of the generic UI component, not the React module! label Oct 1, 2018
@vilvaathibanpb
Copy link
Contributor Author

vilvaathibanpb commented Oct 1, 2018

@eps1lon . The use case - We had a request as follows:

"This would be much better if having clickable={false} OnClick={() => { do something}}. I would like that to disable the click. As of right now, if you have an onclick, clickable does absolutely nothing. The feature that would be much better if styling and making the onclick not function not work. As clickable false, seems like the "chip" should be disabled"

PR Link

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 1, 2018

If anything I think we should warn if users set clickable to false and onClick to a function.

People always want to do crazy stuff. Very often, we are allowing them. @zachrickards would you mind sharing your use case? The issue with the warning is that people can no longer easily have an onClick event listener without the clickable state. Adding a warning is definitely a viable option.

@oliviertassinari oliviertassinari changed the title Issue-#13052 [Chip] Control clickable prop irrespective of onClick Prop [Chip] Control clickable prop irrespective of onClick Prop Oct 1, 2018
@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 1, 2018
@oliviertassinari oliviertassinari self-assigned this Oct 20, 2018
@oliviertassinari oliviertassinari changed the title [Chip] Control clickable prop irrespective of onClick Prop [Chip] Control clickable property Oct 23, 2018
@oliviertassinari oliviertassinari added new feature New feature or request and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 23, 2018
@oliviertassinari oliviertassinari merged commit ffe6ee3 into mui:master Oct 23, 2018
@oliviertassinari
Copy link
Member

@vilvaathibanpb Thank you

@vilvaathibanpb
Copy link
Contributor Author

@oliviertassinari Always welcome 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: chip This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants