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

docs(tooltip): use Button instead of Information icons in stories #17006

Merged

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Jul 19, 2024

A small detail of #12921, carbon-design-system/carbon-design-kit#721, #15887 that was missed, this PR modifies the Tooltip storybook examples to use a button as the trigger to avoid confusion stemming from information icons.

Related to carbon-design-system/carbon-website#3956

Changelog

Changed

  • Tooltip stories now use SquareOutline

Removed

  • Removed an invalid Tooltip example from FormLabel stories, it already had a valid Toggletip example alongside it

Testing / Reviewing

  • Tooltip stories should function and have no regressions

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit f80393a
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/669ab31ce431dc00088e4264
😎 Deploy Preview https://deploy-preview-17006--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit f80393a
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/669ab31cbd2d4f0008df256e
😎 Deploy Preview https://deploy-preview-17006--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit e400716
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/669ab40117087f00081a537a
😎 Deploy Preview https://deploy-preview-17006--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit e400716
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/669ab401d49f2c00084b7e28
😎 Deploy Preview https://deploy-preview-17006--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 86c0de1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/673ba3fad74af10008c7a653
😎 Deploy Preview https://deploy-preview-17006--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 86c0de1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/673ba3fa09b03300085750ef
😎 Deploy Preview https://deploy-preview-17006--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@wkeese wkeese left a comment

Choose a reason for hiding this comment

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

These changes look like an improvement to me, but I think the other issue (according to Michael Gower) is to update https://carbondesignsystem.com/components/tooltip/usage/#standard-tooltip to say that "standard tooltips" should be displayed using Toggletip. Right now it says:

If the content is essential for the user to interpret concerning their workflow, use a toggletip for this information instead.

Actually it seems like you should replace all the text to say "Informational tooltip" wherever it now says "Standard tooltip"?

@tay1orjones
Copy link
Member Author

@wkeese Cool, yeah this is just the actionable piece for the storybook since the website repo is separate.

@kingtraceyj could you share the issue that is tracking the website updates you mentioned?

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

Hey @tay1orjones
The SquareOutline is not defined in the import, I guess that's why the stories are all blank https://deploy-preview-17006--v11-carbon-react.netlify.app/?path=/story/components-tooltip--playground

The FormLabel looks good to me!

guidari
guidari previously approved these changes Aug 15, 2024
@guidari guidari dismissed their stale review August 15, 2024 18:59

Wrong one

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

@tay1orjones The stories doesn't have the <Information /> icon anymore.

I added the <Information /> back in to test the code locally and the focus looks weird.

<Tooltip align="bottom" label={label}>
      <Button className="sb-tooltip-trigger">
        <Information />
      </Button>
    </Tooltip>

Screenshot 2024-08-15 at 16 05 05

@tay1orjones
Copy link
Member Author

@guidari sorry for the back and forth here, I need to update the tooltip styling and correct a few other things. I think that specific focus ring issue could be solved by adding hasIconOnly to the Button.

@alisonjoseph
Copy link
Member

alisonjoseph commented Oct 8, 2024

@tay1orjones I think you'll need to remove the custom styles to fix the weird display of the button in the Tooltip story. /components/Tooltip/story.scss (lines 24-40)

Wasn't sure if there was anything else that needed to be done with this PR

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.23%. Comparing base (1df6d92) to head (86c0de1).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17006      +/-   ##
==========================================
+ Coverage   82.19%   82.23%   +0.04%     
==========================================
  Files         404      404              
  Lines       14151    14184      +33     
  Branches     4418     4505      +87     
==========================================
+ Hits        11631    11664      +33     
+ Misses       2359     2358       -1     
- Partials      161      162       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@tay1orjones
Copy link
Member Author

@alisonjoseph thanks for the heads up! I updated it - this should be ready now

Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit 86c0de1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/673ba3fa4227ed0008b67b88
😎 Deploy Preview https://deploy-preview-17006--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

LGTM!

@alisonjoseph alisonjoseph added this pull request to the merge queue Nov 18, 2024
Merged via the queue into carbon-design-system:main with commit 3aa7596 Nov 18, 2024
40 checks passed
@carbon-automation
Copy link
Contributor

Hey there! v11.71.0 was just released that references this issue/PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants