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

Update Tooltip types to closer align to Radix capabilities #102

Closed
wants to merge 2 commits into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Oct 11, 2023

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 11, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6134ed2
Status: ✅  Deploy successful!
Preview URL: https://038ab60e.compound-web.pages.dev
Branch Preview URL: https://t3chguy-patch-1.compound-web.pages.dev

View logs

@t3chguy t3chguy marked this pull request as ready for review October 11, 2023 14:17
@t3chguy t3chguy requested a review from a team as a code owner October 11, 2023 14:17
@t3chguy t3chguy requested review from weeman1337 and robintown and removed request for a team October 11, 2023 14:17
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

I don't think it's desirable to add more flexibility here. It opens the door to folks adding things that we do not wish to see in tooltips.

Like links, and other things...
Looping @janogarcia as the lead of the design side of Compound

@germain-gg germain-gg requested a review from janogarcia October 11, 2023 14:24
@t3chguy
Copy link
Member Author

t3chguy commented Oct 11, 2023

image
image

the result will be being unable to represent what designs ask of me in Compound tooltips and continuing the use of our home-grown react-sdk ones, which already have similar usages:

image
image

@t3chguy t3chguy marked this pull request as draft October 11, 2023 14:53
Copy link

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

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

I'll try to address all the questions on Compound Tooltips shared here and in internal channels:

  • It should support a primary (high contrast) and a secondary (lower contrast) line of text.
  • Maximum width should be around 200-250px, but I don't think we have formalized that detail yet on the spec. Let's say 240px for now.
  • Text can wrap when the container reaches the maximum size (or when there's forced line breaks), but it is not recommended to display long text on tooltips.
  • I don't think we support text formatting.
  • The basic tooltip component shouldn't include interactive elements (e.g., links, buttons). If we need to support that we should think about a separate behavior/component for accessibility reasons (similarly to what IBM Carbon does for Tooltip, Toggletip, and Popover components, or what Google Material 3 does for Plain and Rich Tooltips).

Let me know if I'm still missing to address some stuff, as I'm not sure what changes this PR would bring.


Relevant resources

@t3chguy t3chguy closed this Oct 11, 2023
@t3chguy
Copy link
Member Author

t3chguy commented Oct 11, 2023

Thanks for the clarification @janogarcia

@janogarcia
Copy link

janogarcia commented Oct 11, 2023

@t3chguy What was this PR introducing, though? Maybe free-form HTML?

@t3chguy
Copy link
Member Author

t3chguy commented Oct 11, 2023

@janogarcia it was passing through Radix's flexibility to contain more than just a single line of text in a tooltip. I had missed that the Compound tooltip had a way to specify a sub line due to it having been called shortcut

to accomplish a

image

but instead I went with

image

Using the shortcut prop for the 2nd line.

@janogarcia
Copy link

@t3chguy Thanks! We should rename that property, though, as it's not strictly for shortcuts. It's just a generic secondary line.

BTW, I see that the current implementation is using a <small> tag for the secondary line that shouldn't be there, as it's making the text ridiculously small.

@t3chguy
Copy link
Member Author

t3chguy commented Oct 11, 2023

@janogarcia indeed, my confusion was I didn't even expect it to be rendered based on its documentation being The associated keyboard shortcut - I presumed it'd be related to actual keyboard handling - https://github.com/vector-im/compound-web/blob/main/src/components/Tooltip/Tooltip.tsx#L31-L38

Agreed also on it being on the small side

@janogarcia
Copy link

@t3chguy Captured those issues in #246

@t3chguy
Copy link
Member Author

t3chguy commented Oct 11, 2023

Thanks @janogarcia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants