-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
[IconButton] Remove on hover effect when disableRipple
is set
#29298
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we need some updates sorry :)
Made your requested updates @mnajdova |
I made a couple more changes, can you take a look when you get the chance? @mnajdova |
Hey @mnajdova is this good to go? |
Hey @mnajdova bumping this one more time in an attempt to make my Hacktoberfest goal before the deadline. Hoping you can give it another look :) |
So sorry was a bit busy, the notification must have slipped. All good from my end. |
disableRipple
is set
the version of 5.8.6 can works now? |
</IconButton>, | ||
); | ||
expect(container.querySelector('.touch-ripple')).to.equal(null); | ||
expect(getComputedStyle(getByRole('button'), ':hover').backgroundColor).to.equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This :hover
argument is not valid for the getComputedStyle(element, pseudoElt)
API. The second parameter expects a pseudo-element, not a pseudo-class, see https://stackoverflow.com/questions/11495535/why-doesnt-getcomputedstyle-work-with-pseudo-classes-like-hover.
It seems that we can't test this. I'm removing the test in: #34764 as it's always true.
Added a check for the
disableRipple
prop before adding the on-hover background color styling.Addresses #11108