[Chip] clickable={false} option still executes onClick method

[asjustis]

Hi,

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

I am trying to conditionally render my Chip component to be clickable or read-only. When I set clickable={false}, it still executes onClick when pressed.

Expected Behavior 🤔

I would naturally expect that both UI and the method execution would be disabled by the clickable={false} option.

Steps to Reproduce 🕹

I added a quick sandbox example here to reproduce: https://codesandbox.io/s/vigilant-water-2yygy?file=/src/App.js

Steps:

1. add onClick method
2. add clickable={false} line
3. click on the tag in the UI
4. it still executes the onClick() method

Your Environment 🌎

Using latest 4.11.3 core version:

    OS: macOS Mojave 10.14.5
  Binaries:
    Node: 10.21.0 - /usr/local/bin/node
    Yarn: Not Found
    npm: 6.14.4 - /usr/local/bin/npm
  Browsers:
    Chrome: 88.0.4324.96
    Edge: Not Found
    Firefox: 83.0
    Safari: 12.1.1
  npmPackages:
    @material-ui/core: ^4.11.3 => 4.11.3 
    @material-ui/icons: ^4.5.1 => 4.5.1 
    @material-ui/lab: ^4.0.0-alpha.38 => 4.0.0-alpha.38 
    @material-ui/styles:  4.11.3 
    @material-ui/system:  4.11.3 
    @material-ui/types:  5.1.0 
    @material-ui/utils:  4.7.1 
    @types/react:  16.9.11 
    react: ^16.9.0 => 16.9.0 
    react-dom: ^16.9.0 => 16.9.0 

[oliviertassinari]

@asjustis Interesting, the clickable prop has the following description:

If true, the chip will appear clickable, and will raise when pressed, even if the onClick prop is not defined. If false, the chip will not be clickable, even if onClick prop is defined. This can be used, for example, along with the component prop to indicate an anchor Chip is clickable.

I don't think that we should implement the behavior proposed because clickable is meant to change the UI, not the behavior. I would also be worried about the breaking change without any easy workaround (as you have now). I vote for "won't fix".

[asjustis]

@oliviertassinari ah, I see, agreed that it would most likely be a breaking change. The name clickable suggests it controls the Chip component itself, rather than the UI.

If false, the chip will not be clickable, even if onClick prop is defined

It seems, this part in documentation might be misleading. Maybe a note in documentation would help to clarify this? Note: this controls the UI and does not affect the onClick event

diff --git a/packages/material-ui/src/Chip/Chip.d.ts b/packages/material-ui/src/Chip/Chip.d.ts
index cab738ff63..d9a6b2b480 100644
--- a/packages/material-ui/src/Chip/Chip.d.ts
+++ b/packages/material-ui/src/Chip/Chip.d.ts
@@ -89,9 +89,10 @@ export interface ChipTypeMap<P = {}, D extends React.ElementType = 'div'> {
     /**
      * If `true`, the chip will appear clickable, and will raise when pressed,
      * even if the onClick prop is not defined.
-     * If false, the chip will not be clickable, even if onClick prop is defined.
+     * If `false`, the chip will not be clickable, even if onClick prop is defined.
      * This can be used, for example,
      * along with the component prop to indicate an anchor Chip is clickable.
+     * Note: this controls the UI and does not affect the onClick event.
      */
     clickable?: boolean;
     /**

[oliviertassinari]

This clarification sounds great, do you want to update the docs in next?

[asjustis]

@oliviertassinari Ah didn't get back in time. Thanks for closing this!