-
Notifications
You must be signed in to change notification settings - Fork 1.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
fix(button): conflicting hover state #9504
fix(button): conflicting hover state #9504
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: 3be5cd4 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/612fc0bce995ff0007985d8f 😎 Browse the preview: https://deploy-preview-9504--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 7051081 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/611e62303367890007722ff4 😎 Browse the preview: https://deploy-preview-9504--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 7051081 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/611e623041c026000972083a 😎 Browse the preview: https://deploy-preview-9504--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 3be5cd4 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/612fc0bcd1a63300080b8421 😎 Browse the preview: https://deploy-preview-9504--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 3be5cd4 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/612fc0bc815fe3000801defa 😎 Browse the preview: https://deploy-preview-9504--carbon-components-react.netlify.app |
This reverts commit e44f5e8.
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.
Looks great, just a few comments! I think TooltipIcon
uses the same logic, so I think we'd want to port the same fixes over there as well. Do you want to bundle that in this PR, or create a new one?
Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: TJ Egan <[email protected]>
… 8961-mouseout-alt
@tw15egan Removed those zombies and removed the logic from |
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.
LGTM once test stories are removed 👍 ✅
Closes #8961
Refs #7040
Refs #4560
Refs #8022
*** DO NOT MERGE** until the
MouseOut
logic is removed from the Button story.By removing the
setIsHovered
state on handleFocus and handleMouseEnter, I was able to isolate and remove the conflicting style,${prefix}--tooltip--visible
, fixing the issue.Also removed the conflicting Storybook Actions add-on for the Button story.
Testing / Reviewing
Open storybook and examine Button, specifically
iconOnly
button, the Playground, and theMouseOut
story (the story that needs to be removed before merging the PR). Ensure the tooltips on the button fire on/off correctly with mouse and keyboard control.