-
Notifications
You must be signed in to change notification settings - Fork 77
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
Component: Tooltips #164
Component: Tooltips #164
Conversation
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.
Notes.
type updates, overall spacing and image placement
@driskull, the tooltip will only be the message on hover. We should move the interaction versions (click, close X, image, button, etc) to the popover. So tooltip can only have the text message and evoked on hover. Similar to a discussion we had. |
Thanks @julio8a. I'll proceed with that. |
Updated PR. Please review |
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.
Some tiny nitpicks, looks great!
Moves font size declaration to container, adds component prefix, removes unused classes.
Small update to css / clean classes no longer in use. |
Is this accessible? Looks like there isn't anything tying the popover to the triggered element. So if I put a popover on the screen in a different location, screen readers will have no way of understanding what element it describes. |
Consider a list of folders with buttons on each folder for edit/delete. If the popovers are placed outside the list so they aren't screwed up by overflow hidden, the screen reader will read "edit, delete, edit, delete, edit, delete, edit, delete, edit, delete" and then the names of the folders. I think we should be using |
We should add some aria roles to both components for sure. We could hold off or address in follow up issues. |
popovers most likely should be managed with aria-live regions so that screen-readers announce their content |
Follow up is fine by me, but we should capture in an issue to make sure we don't forget 👍 |
What's the level of effort and time estimate? I feel that if we put it in an issue, it'll still take a few weeks or more to be taken care of, or when a user stumbles on the accessibility issue. |
I can tackle it later today. |
closing in favor of #167 167 |
calcite-tooltip
Properties
open
open
boolean
false
placement
placement
"auto" | "auto-end" | "auto-start" | "bottom" | "bottom-end" | "bottom-start" | "left" | "left-end" | "left-start" | "right" | "right-end" | "right-start" | "top" | "top-end" | "top-start"
"auto"
referenceElement
reference-element
HTMLElement | string
undefined
theme
theme
"dark" | "light"
"light"
Methods
reposition() => Promise<void>
Returns
Type:
Promise<void>
#155 (comment)