-
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
feat(react): add toggletip component #10365
feat(react): add toggletip component #10365
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: 32152dd 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61f0642987d7e700070c3660 😎 Browse the preview: https://deploy-preview-10365--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 32152dd 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61f06429d43b8b00070d350c 😎 Browse the preview: https://deploy-preview-10365--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 32152dd 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61f064294aaa57000944505a 😎 Browse the preview: https://deploy-preview-10365--carbon-components-react.netlify.app |
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 good to me! Composing ToggletipLabel
outside of Toggletip
looks a bit strange but I like the preference towards flexibility.
Co-authored-by: Taylor Jones <[email protected]>
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.
- The hover state for the icon looks like its
$icon-primary
, so that is correct. 👍
-
There should be 8px between the top of the caret tip and info icon.
-
The focus on the link and button inside of the popover container need to be using
$focus-inverse
.
- Also just a question, are direction for top, bottom, left, and right going to be provided eventually? Or should they be a part of this PR.
…280-add-toggletip-component
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 good to me!!
…carbon into 10280-add-toggletip-component
@laurenmrice thanks for the review! Just updated with the changes. I also included a "playground" story with the directions (although the "top" option will run into the top of the page, will figure out a good way to center stories in a future PR). Separately, it looked like the button was using the correct token but let me know if you still are seeing that problem in the latest update! |
@joshblack The $focus-inverse looks good in the light themes. But for the dark themes, the button still gets incorrect focus styling. The focus around the button should be blue and not white. |
@laurenmrice great point, thanks! I'll make an update today |
@laurenmrice just pushed up the change so that the button uses |
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.
It looks good to me now! 👍
Side note: Eventually design is going to add another focus color token for $focus-inset that is inverse for these situations when a button appears on an inverse background. It will help improve some of the color contrast.
Closes #10280
Adds the toggletip set of components to this project. This is the initial API Design for
Toggletip
and its corresponding components. It includes test stubs but no tests yet for the implementation while we go through API review 👀Changelog
New
useWindowEvent
touseEvent
Changed
useEvent
to supports refsRemoved
Testing / Reviewing