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

Component: Tooltips #164

Closed
wants to merge 34 commits into from
Closed

Component: Tooltips #164

wants to merge 34 commits into from

Conversation

driskull
Copy link
Member

@driskull driskull commented Oct 3, 2019

calcite-tooltip

Properties

Property Attribute Description Type Default
open open Display and position the component. boolean false
placement placement Determines where the component will be positioned relative to the referenceElement. "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 Reference HTMLElement used to position this component. HTMLElement | string undefined
theme theme Select theme (light or dark) "dark" | "light" "light"

Methods

reposition() => Promise<void>

Returns

Type: Promise<void>


#155 (comment)

@driskull driskull added help wanted Issues that the core team needs help with in a sprint. new component Issues tied to a new component. docs Issues relating to documentation updates only. design Issues that need design consultation prior to development. labels Oct 3, 2019
@driskull driskull self-assigned this Oct 3, 2019
Copy link
Member Author

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Notes.

@julio8a
Copy link

julio8a commented Oct 7, 2019

@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.

@driskull
Copy link
Member Author

driskull commented Oct 8, 2019

Thanks @julio8a. I'll proceed with that.

@driskull
Copy link
Member Author

driskull commented Oct 9, 2019

Updated PR. Please review

Copy link
Contributor

@macandcheese macandcheese left a 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.
@macandcheese
Copy link
Contributor

Small update to css / clean classes no longer in use.

@macandcheese
Copy link
Contributor

Good to merge this in? @driskull @julio8a

@paulcpederson
Copy link
Member

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.

@paulcpederson
Copy link
Member

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 role="tooltip" and aria-describedby to connect them, but I could be wrong

@driskull
Copy link
Member Author

We should add some aria roles to both components for sure. We could hold off or address in follow up issues.

@paulcpederson
Copy link
Member

popovers most likely should be managed with aria-live regions so that screen-readers announce their content

@paulcpederson
Copy link
Member

Follow up is fine by me, but we should capture in an issue to make sure we don't forget 👍

@julio8a
Copy link

julio8a commented Oct 10, 2019

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.

@driskull
Copy link
Member Author

I can tackle it later today.

@driskull
Copy link
Member Author

closing in favor of #167 167

@driskull driskull closed this Oct 10, 2019
@driskull driskull deleted the dris0000/calcite-tooltip branch October 10, 2019 22:50
@macandcheese macandcheese added this to the 🚢 v1-beta.11 milestone Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Issues that need design consultation prior to development. docs Issues relating to documentation updates only. help wanted Issues that the core team needs help with in a sprint. new component Issues tied to a new component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants