-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(web-components): add Tooltip component #32852
base: master
Are you sure you want to change the base?
Conversation
📊 Bundle size report✅ No changes found |
🕵 fluentui-web-components-v3 No visual regressions between this PR and main |
* @public | ||
*/ | ||
export const template = html<Tooltip>` | ||
<template id="tooltip-${x => x.anchor}" popover aria-hidden="true"> |
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.
Does this get properly overridden if someone wants to assign a specific id attribute?
public connectedCallback(): void { | ||
super.connectedCallback(); | ||
if (this.anchorElement) { | ||
this.anchorElement.setAttribute('aria-describedby', `tooltip-${this.anchor}`); |
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.
Same question as with the template: what happens if the id
of the tooltip is overridden?
font-size: ${fontSizeBase200}; | ||
inset-area: block-start; | ||
line-height: ${lineHeightBase200}; | ||
margin: 0; |
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.
Does this need to be set?
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.
Good question, yes it does. [popover]
applies a margin: auto
in the default stylesheet. So we need this to unset
that value otherwise it tries to center itself. Three options:
- Keep as
margin: 0
- Update to
margin: unset
for clarity - Create two declarations to explicitly
unset
in the:host(:is([positioning^...
rules.
box-sizing: border-box; | ||
color: ${colorNeutralForeground1}; | ||
display: inline-flex; | ||
filter: drop-shadow(0 0 2px ${colorNeutralShadowAmbient}) drop-shadow(0 4px 8px ${colorNeutralShadowKey}); |
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.
Is there a preference for filter
over box-shadow
?
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.
A little future-proofing, but filter
will work with the arrow better by going around that shape while box-shadow only does the box. Also keeping parity with the React implementation here.
line-height: ${lineHeightBase200}; | ||
margin: 0; | ||
max-width: 240px; | ||
padding: 4px 11px 6px; |
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.
Does 11px come from design? Or the react component? Seems out of place
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.
Comes from the React component. The padding-block-*
values seem to be font-metric based but yea, not sure where 11px comes from in a token sense. If we wanted to tokenize we could move it up or down 1px.
mNudge: '10px',
m: '12px',
Previous Behavior
New Behavior
positioning
shorthandsNote
Needs feedback! Tooltip requires CSS anchor position polyfill for older browsers but polyfill is still in-progress.
Related Issue(s)