-
Notifications
You must be signed in to change notification settings - Fork 353
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
visual design in interface guidelines + accessibility guidelines
#565
Conversation
alt="tooltips rendered in all possible positions surrounding a control" | ||
/> | ||
|
||
## Alternatives to tooltips |
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.
Love this section so much
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.
First pass! Thank you for working on this :)
I hope to take a closer look tomorrow!
alt="tooltips rendered in all possible positions surrounding a control" | ||
/> | ||
|
||
## Alternatives to tooltips |
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.
We have a couple of lint rules and doc pages where we cross-reference the existing Tooltip alternatives docs. Let's make sure to update those to avoid a dead link! (I can verify tomorrow which ones those are).
Or could it be sufficient to just cross-reference that page from this section rather than moving it entirely?
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.
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.
I'll follow up with the links outside this repo once this PR merges! ❤️ thank you
|
||
Most UI cases don't call for tooltips. Learn some [alternative methods to use in place of tooltips](/../guides/accessibility/tooltip-alternatives). | ||
## Additional resources |
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.
We introduced a new PVC lint rule recently the deprecated Primer CSS tooltipped which we have a ton of debt around that we want to tackle. It looks like the lint rule doc is now on Primer CSS Tooltipped.
Could it make sense to link to it here? :)
|
||
## Options | ||
|
||
### Label |
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.
Could it be useful to talk about the underlying markup of the differences between a label
and description
(e.g. aria-labelledby
vs aria-describedby
)? One will serve as the accessible name of a control, while the other will supplement it.
We have some more documentation in Primer Rails tooltip (This was originally under an accessibility section in Primer Rails but I think it changed with the recent docs site update!)
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.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
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.
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people 🤔
(cc: @broccolinisoup who worked on React tooltip :) )
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.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
Katie let me know if you want to pair on this or any help with it!
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people
Yes! We are going to ship the draft version soon hopefully 🙏🏻 and also the visual changes (i.e. removing the caret) also will be shipped with the draft version so holding off merging this PR until the draft is out would be wise.
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.
😍
Hey @langermank could you add a Figma section as well? |
alt="tooltips rendered in all possible positions surrounding a control" | ||
/> | ||
|
||
## Alternatives to tooltips |
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.
|
||
## Options | ||
|
||
### Label |
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.
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people 🤔
(cc: @broccolinisoup who worked on React tooltip :) )
content/components/tooltip.mdx
Outdated
For interactive controls that require additional context, a tooltip can be used to provide it. The `label` is also | ||
required, and the description will be announced to a screen reader in addition to the label. Keep the description |
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.
I think we will want to reword this a bit since this seems to suggest one should set both the label
and description
prop of the tooltip API which I don't think is the case. Maybe we want to say that when a tooltip supplements a control as a description, we should make sure the control has an accessible name?
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>
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.
we should make sure the control has an accessible name?
I agree with this. Since accessible name could be a visible button text or an aria-label
, we can maybe divide the "Description" section into two subsections?
Description for buttons that has a visible name
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>
Description for buttons that doesn't have a visible name
<iconbutton aria-label="icon-button-label" />
<tooltip id="tooltip-1">Some supplementary description</tooltip>
|
||
## Options | ||
|
||
### Label |
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.
Could we mention how a label type tooltip should be reserved for icon buttons? The underlying semantic of a label tooltip is aria-labelledby
would override any label the button already has.
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.
I feel like this is clear already by saying "For interactive controls with no visible text label" but I will add "such as icon buttons" 😄
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.
So cool 🔥 Left some comments, let me know what you think 🙌🏻
|
||
## Options | ||
|
||
### Label |
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.
Yes, thank you! I had a hard time trying to explain this one so let me see if I can rework it a bit.
Katie let me know if you want to pair on this or any help with it!
I don't think this is shipped in React. Would we want to hold off on this section to avoid confusing people
Yes! We are going to ship the draft version soon hopefully 🙏🏻 and also the visual changes (i.e. removing the caret) also will be shipped with the draft version so holding off merging this PR until the draft is out would be wise.
content/components/tooltip.mdx
Outdated
For interactive controls that require additional context, a tooltip can be used to provide it. The `label` is also | ||
required, and the description will be announced to a screen reader in addition to the label. Keep the description |
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.
we should make sure the control has an accessible name?
I agree with this. Since accessible name could be a visible button text or an aria-label
, we can maybe divide the "Description" section into two subsections?
Description for buttons that has a visible name
<button aria-describedby="tooltip-1">Name of button</button>
<tooltip id="tooltip-1">Some supplementary description</tooltip>
Description for buttons that doesn't have a visible name
<iconbutton aria-label="icon-button-label" />
<tooltip id="tooltip-1">Some supplementary description</tooltip>
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
…into update-tooltip-docs
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
…into update-tooltip-docs
Goals
Accessibility page → https://primer-14e523666e-26441320.drafts.github.io/guides/accessibility/tooltip-alternatives
Interface guidelines → https://primer-14e523666e-26441320.drafts.github.io/components/tooltip
TODO