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

feat(DefinitionTooltip): add Definition Tooltip #10262

Merged

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Dec 9, 2021

Closes #9902
Closes #8814
Closes #6429
Refs #9901

(merged the last PR #10197 on accident too soon. So reverted that and HERE WE GO AGAIN)

This PR adds the DefinitionTooltip component (utilizing our Popover primitive 🍖) and DefinitionTooltip styles ⚡
image

Testing / Reviewing

In the carbon-react package run the storybook and check out the unstable_tooltip > Definition story. Make sure it matches intended spec and behavior. 🏄🏾

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, only comment would be to move tests over into its own test file 👍

@netlify
Copy link

netlify bot commented Dec 9, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 2e86f04

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61f1d7965309920007bd5217

😎 Browse the preview: https://deploy-preview-10262--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Dec 9, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 2e86f04

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61f1d7967fddae000729cdc9

😎 Browse the preview: https://deploy-preview-10262--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 9, 2021

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 2e86f04

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61f1d7960e29240007a86380

😎 Browse the preview: https://deploy-preview-10262--carbon-components-react.netlify.app

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakahn

Hover:

  • The dotted underline should be color token $border-interactive.
    Artboardblue

Popover:
Caret tip should be 16px to the right. This will be tackled in a separate PR for Popover. Definition tooltip will inherit the change.
Artboard

Safari bug:

  • When clicking on the trigger text, the text disappears briefly and there is no focus box.
    safari click bug

Theme switching:
When switching themes, there a problem with the theme switcher It seems like the p text is using the correct color token $text-primary, but it is visually not showing up correctly when I change themes. The URL trigger text also appears to be the wrong color.
Screen Shot 2021-12-09 at 10 32 41 AM

On click option:

  • Can we add a prop to have the option to make the tooltip appear on click instead of on hover? We have heard some teams prefer to have the definition tooltip appear on click because of their product usecases. We would want to keep the hovering interaction as our default still, but would like to add an option for to open the tooltip on click.

@joshblack
Copy link
Contributor

@laurenmrice for this point:

Caret tip should be 16px to the right.

Is this something specific for definition tooltip? I believe the reason that it's centered is that we made that update to popover so that the caret is placed center of the target so just wanted to confirm.

When switching themes, is there a problem with the theme switcher?

I think this should be fixed in latest, I believe we didn't have color: $text-primary on the body for the default color 👍

@laurenmrice
Copy link
Member

@joshblack I just double checked the PR of the popover refactor and I missed in that visual review that the caret tip is not aligned to the 16px padding. It should be aligned to the type that is following the 16px padding and the caret tip should still be vertically center with the trigger button. So maybe this is something that needs to be fixed for Popover and then inherited for Definition tooltip?

Example alignment:
Artboard 1

@joshblack
Copy link
Contributor

So maybe this is something that needs to be fixed for Popover and then inherited for Definition tooltip?

Definitely, that makes sense to me 👍

@dakahn dakahn requested a review from laurenmrice January 14, 2022 00:09
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakahn

Enabled (closed) state

  • The dotted line needs to use color token $border-strong for the Enabled (closed) state. On Hover and Active states it changes to color token $border-interactive.
    Artboard

Theming

  • The text color of the definition tooltip in the sentence is showing up as the wrong color in the dark themes. The text color should be using color token $text-primary.

Example:
Screen Shot 2022-01-18 at 10 58 34 AM

My previous comment about having an “on click option”

Can we add a prop to have the option to make the tooltip appear on click instead of on hover? We have heard some teams prefer to have the definition tooltip appear on click because of their product usecases. We would want to keep the hovering interaction as our default still, but would like to add an option for to open the tooltip on click.

Is this going to be tackled in a separate PR?

@dakahn
Copy link
Contributor Author

dakahn commented Jan 18, 2022

@laurenmrice i should be able to get that prop in. Thanks for the review 🏄🏾

@dakahn dakahn requested a review from laurenmrice January 20, 2022 00:48
@dakahn
Copy link
Contributor Author

dakahn commented Jan 20, 2022

@laurenmrice added the openOnClick prop. I've updated the storybook story to use it by default for testing 👍🏾

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openOnClick prop:

  • Even though you have to click on the text to open the tooltip, there should still be a hover color change of the underline. The color token on hover should be $border-interactive.
    hover color

Safari bug:

  • Once the definition tooltip is open, you can not close the tooltip by clicking anywhere on the page and you should be able to.

tooltip close

  • Everything else looks good! 👍

@dakahn
Copy link
Contributor Author

dakahn commented Jan 24, 2022

@laurenmrice I can recreate the Safari bug, but I'm not sure whats causing it. For other dev reviewers, it look like state is being set properly and onBlur is being called as expected 😕

@dakahn dakahn requested a review from laurenmrice January 25, 2022 20:52
@tw15egan
Copy link
Collaborator

In my past experience, sometimes the onBlur doesn't trigger in Safari since button clicks technically don't give the element focus in Safari. I think you may need to manually focus the element in the onClick

onClick={(evt) => {
    evt.target.focus();
    setOpen(!isOpen);
}}

I can't test this, because mine works in Safari for some reason 🤷🏻‍♂️

2022-01-26 09 34 33

Copy link
Member

@laurenmrice laurenmrice left a 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 now! Thanks D.A.! 🎉

@joshblack
Copy link
Contributor

Anything left for this one @dakahn? 👀

@dakahn
Copy link
Contributor Author

dakahn commented Jan 26, 2022

@joshblack yep! just a couple broken tests im patchin up rn. should be ready to go in the next hour tho 👍🏾

@kodiakhq kodiakhq bot merged commit 1d2fe39 into carbon-design-system:main Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants