-
Notifications
You must be signed in to change notification settings - Fork 13
Add Tooltip stories for Links and Rich Text Content #2995
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
base: main
Are you sure you want to change the base?
Changes from all commits
69b3c2a
2af3353
60d4c32
423216e
0fccf04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| --- | ||
| --- |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,12 +8,17 @@ import magnifyingGlass from "@phosphor-icons/core/regular/magnifying-glass.svg"; | |||||||
| import info from "@phosphor-icons/core/regular/info.svg"; | ||||||||
|
|
||||||||
| import Button from "@khanacademy/wonder-blocks-button"; | ||||||||
| import Link from "@khanacademy/wonder-blocks-link"; | ||||||||
| import {PropsFor, View} from "@khanacademy/wonder-blocks-core"; | ||||||||
| import {TextField} from "@khanacademy/wonder-blocks-form"; | ||||||||
| import IconButton from "@khanacademy/wonder-blocks-icon-button"; | ||||||||
| import {OnePaneDialog, ModalLauncher} from "@khanacademy/wonder-blocks-modal"; | ||||||||
| import {semanticColor, spacing} from "@khanacademy/wonder-blocks-tokens"; | ||||||||
| import {Body} from "@khanacademy/wonder-blocks-typography"; | ||||||||
| import { | ||||||||
| semanticColor, | ||||||||
| sizing, | ||||||||
| spacing, | ||||||||
| } from "@khanacademy/wonder-blocks-tokens"; | ||||||||
| import {BodyText} from "@khanacademy/wonder-blocks-typography"; | ||||||||
| import {PhosphorIcon} from "@khanacademy/wonder-blocks-icon"; | ||||||||
|
|
||||||||
| import Tooltip from "@khanacademy/wonder-blocks-tooltip"; | ||||||||
|
|
@@ -164,6 +169,70 @@ export const ComplexAnchorAndTitle: StoryComponentType = { | |||||||
| }, | ||||||||
| }; | ||||||||
|
|
||||||||
| /** | ||||||||
| * Tooltips can be used with links as anchors. | ||||||||
| * When a `Link` is the anchor element, set `forceAnchorFocusivity={false}` | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Focusivity isn't exactly a word but it works! (I thought it would be "forceAnchorFocusability" when trying to write from memory) Updated the argtype.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I invented that term a long time ago. |
||||||||
| * since the link is already keyboard focusable. The tooltip will appear on | ||||||||
| * hover or focus and the `aria-describedby` attribute is automatically applied | ||||||||
| * to the `Link` element. | ||||||||
| */ | ||||||||
| export const WithLinkAnchor: StoryComponentType = { | ||||||||
| render: function Render() { | ||||||||
| return ( | ||||||||
| <Tooltip | ||||||||
| content="This link navigates to the Khan Academy homepage." | ||||||||
| placement="top" | ||||||||
| forceAnchorFocusivity={false} | ||||||||
| > | ||||||||
| <Link href="https://www.khanacademy.org">Khan Academy</Link> | ||||||||
| </Tooltip> | ||||||||
| ); | ||||||||
| }, | ||||||||
|
Comment on lines
169
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The Extended reasoning...What the bug is and how it manifests The The specific code path that triggers it In <Tooltip
content="This link navigates to the Khan Academy homepage."
placement="top"
forceAnchorFocusivity={false}
>
<Link href="https://www.khanacademy.org">Khan Academy</Link>
</Tooltip>There is no Why existing code doesn't prevent it Every other story in this file that needs the tooltip bubble visible uses one of three strategies: (1) Impact CLAUDE.md (line 244) explicitly states: "Disable Chromatic for stories that don't need visual regression tests (limited monthly snapshots)." A snapshot that captures only a bare link element with no tooltip visible provides no visual regression value and burns a monthly snapshot. The story's own JSDoc says it demonstrates "Tooltips can be used with links as anchors", but the captured snapshot shows no tooltip at all. Step-by-step proof
How to fix it The simplest fix matching this PR's own pattern is to add
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! It caught something I noticed from checking the snapshots visually https://www.chromatic.com/test?appId=5e1bf4b385e3fb0020b7073c&id=69cea98f30ca01a1a71c9dd9 |
||||||||
| play: async ({canvasElement}) => { | ||||||||
| // Arrange | ||||||||
| // NOTE: Using `body` here to work with React Portals. | ||||||||
| const canvas = within(canvasElement.ownerDocument.body); | ||||||||
|
|
||||||||
| // Act | ||||||||
| const link = await canvas.findByRole("link", {name: "Khan Academy"}); | ||||||||
| await userEvent.hover(link); | ||||||||
|
|
||||||||
| // Assert | ||||||||
| await expect( | ||||||||
| await canvas.findByText( | ||||||||
| "This link navigates to the Khan Academy homepage.", | ||||||||
| ), | ||||||||
| ).toBeInTheDocument(); | ||||||||
| }, | ||||||||
| }; | ||||||||
|
|
||||||||
| /** | ||||||||
| * To render rich text in tooltip content, pass a React element as the `content` | ||||||||
| * prop instead of a plain string. When a string is passed it is rendered as | ||||||||
| * plain text — HTML tags in a string will appear literally (e.g. | ||||||||
| * `<i>text</i>`). Use inline HTML elements inside a typography component to | ||||||||
| * control formatting. | ||||||||
| */ | ||||||||
| export const WithRichTextContent: StoryComponentType = { | ||||||||
| render: function Render() { | ||||||||
| return ( | ||||||||
| <Tooltip | ||||||||
| content={ | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if
It wasn't immediately clear to me in the docs how cc: @jandrade in case you have context on this!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did it this way to mimic the production code, so we could validate it works with the setup in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahh hmmm I think this is another case where the Flow to TS migration couldn't add full type safety support for these cases with
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's too bad we can't use TS to limit what components can be used for specific props 😢 (I remember asking about this too in the typescript channel (related thread)) I think keeping the type as |
||||||||
| <BodyText style={{padding: sizing.size_120}}> | ||||||||
| Use <strong>bold</strong>, <em>italic</em>, or{" "} | ||||||||
| <u>underlined</u> text by passing a React element | ||||||||
| instead of a plain string. | ||||||||
| </BodyText> | ||||||||
| } | ||||||||
| opened={true} | ||||||||
| forceAnchorFocusivity={false} | ||||||||
| > | ||||||||
| <Link href="https://www.khanacademy.org">Khan Academy</Link> | ||||||||
| </Tooltip> | ||||||||
| ); | ||||||||
| }, | ||||||||
| }; | ||||||||
|
|
||||||||
| /** | ||||||||
| * In this example, we have the anchor in a scrollable parent. Notice how, when | ||||||||
| * the anchor is focused but scrolled out of bounds, the tooltip disappears. | ||||||||
|
|
@@ -173,7 +242,7 @@ export const AnchorInScrollableParent: StoryComponentType = { | |||||||
| return ( | ||||||||
| <View style={styles.scrollbox}> | ||||||||
| <View style={styles.hostbox}> | ||||||||
| <Body> | ||||||||
| <BodyText> | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for updating typography components! :) |
||||||||
| This is a big long piece of text with a | ||||||||
| <Tooltip | ||||||||
| content="This tooltip will disappear when scrolled out of bounds" | ||||||||
|
|
@@ -182,7 +251,7 @@ export const AnchorInScrollableParent: StoryComponentType = { | |||||||
| [tooltip] | ||||||||
| </Tooltip>{" "} | ||||||||
| in the middle. | ||||||||
| </Body> | ||||||||
| </BodyText> | ||||||||
| </View> | ||||||||
| </View> | ||||||||
| ); | ||||||||
|
|
||||||||

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 comment can be updated!