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

disableFocusRipple doesn't work in iconButton #20941

Closed
matteo1976 opened this issue May 7, 2020 · 4 comments · Fixed by #21116
Closed

disableFocusRipple doesn't work in iconButton #20941

matteo1976 opened this issue May 7, 2020 · 4 comments · Fixed by #21116
Labels
component: icon button This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@matteo1976
Copy link

disableFocusRipple doesn't work in iconButton
I saw that the problem should have been solved here
#15864
but it doesn't really work.
disableRipple works but disableFocusRipple not
here the code:
https://codesandbox.io/s/material-demo-crpxh

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

Current Behavior 😯

Expected Behavior 🤔

Steps to Reproduce 🕹

https://codesandbox.io/s/material-demo-crpxh
Steps:

1.mouseHover on the icon
2.
3.
4.

Context 🔦

Your Environment 🌎

Mac Catalina 10.15.4

Tech Version
Material-UI v4.9.13
React
Browser chrome Version 81.0.4044.138 (Official Build) (64-bit)
TypeScript
etc.
@eps1lon
Copy link
Member

eps1lon commented May 7, 2020

but it doesn't really work.

What would you expect from disableFocusRipple? As far as I can tell this does remove the focus ripple.

@oliviertassinari oliviertassinari added component: icon button This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement labels May 7, 2020
@oliviertassinari
Copy link
Member

The behavior looks all right. However, looking at the description of the prop, I'm a bit confused, shouldn't we do?

diff --git a/packages/material-ui-lab/src/ToggleButton/ToggleButton.js b/packages/material-ui-lab/src/ToggleButton/ToggleButton.js
index bc9f856a0..390cdc7af 100644
--- a/packages/material-ui-lab/src/ToggleButton/ToggleButton.js
+++ b/packages/material-ui-lab/src/ToggleButton/ToggleButton.js
@@ -141,7 +141,6 @@ ToggleButton.propTypes = {
   disabled: PropTypes.bool,
   /**
    * If `true`, the  keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**
diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index fe392b926..9b78e1bb9 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -356,7 +356,6 @@ Button.propTypes = {
   disableElevation: PropTypes.bool,
   /**
    * If `true`, the  keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**
diff --git a/packages/material-ui/src/ButtonGroup/ButtonGroup.js b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
index b51cd3949..d41c1e506 100644
--- a/packages/material-ui/src/ButtonGroup/ButtonGroup.js
+++ b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
@@ -270,7 +270,6 @@ ButtonGroup.propTypes = {
   disableElevation: PropTypes.bool,
   /**
    * If `true`, the button keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**
diff --git a/packages/material-ui/src/Fab/Fab.js b/packages/material-ui/src/Fab/Fab.js
index 86574087f..aa18b740b 100644
--- a/packages/material-ui/src/Fab/Fab.js
+++ b/packages/material-ui/src/Fab/Fab.js
@@ -189,7 +189,6 @@ Fab.propTypes = {
   disabled: PropTypes.bool,
   /**
    * If `true`, the  keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**
diff --git a/packages/material-ui/src/IconButton/IconButton.js b/packages/material-ui/src/IconButton/IconButton.js
index 1bb36a0a0..7a35d5da9 100644
--- a/packages/material-ui/src/IconButton/IconButton.js
+++ b/packages/material-ui/src/IconButton/IconButton.js
@@ -171,7 +171,6 @@ IconButton.propTypes = {
   disabled: PropTypes.bool,
   /**
    * If `true`, the  keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**
diff --git a/packages/material-ui/src/Tab/Tab.js b/packages/material-ui/src/Tab/Tab.js
index 32e38d455..f7ccc838b 100644
--- a/packages/material-ui/src/Tab/Tab.js
+++ b/packages/material-ui/src/Tab/Tab.js
@@ -174,7 +174,6 @@ Tab.propTypes = {
   disabled: PropTypes.bool,
   /**
    * If `true`, the  keyboard focus ripple will be disabled.
-   * `disableRipple` must also be true.
    */
   disableFocusRipple: PropTypes.bool,
   /**

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. and removed support: question Community support but can be turned into an improvement labels May 8, 2020
@umairfarooq44
Copy link
Contributor

@oliviertassinari I am going to make PR for this fix. Is it ok?

@oliviertassinari
Copy link
Member

@umairfarooq44 Definitely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon button This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants