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

Fix empty tooltip and missing required tooltip #770

Merged
merged 5 commits into from Jun 20, 2019
Merged

Conversation

jgreben
Copy link
Contributor

@jgreben jgreben commented Jun 18, 2019

Fixes #757 and #392

@jgreben jgreben moved this from Done to Needs Review in Sinopia Work-Cycle One Jun 19, 2019
@jgreben jgreben changed the title Fix empty tooltip Fix empty tooltip and missing required tooltip Jun 19, 2019
Sinopia Work-Cycle One automation moved this from Needs Review to In Progress Jun 19, 2019
Copy link
Contributor

@ndushay ndushay left a comment

Choose a reason for hiding this comment

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

LGTM, modulo Justin's question (which I don't have strong feelings about)

Sinopia Work-Cycle One automation moved this from In Progress to Needs Review Jun 19, 2019
Copy link
Contributor

@jcoyne jcoyne left a comment

Choose a reason for hiding this comment

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

Screen Shot 2019-06-19 at 8 48 04 AM

This doesn't seem easy to read. I don't think we want both a title and a popover menu. Maybe @astridu can help?

@jgreben
Copy link
Contributor Author

jgreben commented Jun 19, 2019

Screen Shot 2019-06-19 at 8 48 04 AM

This doesn't seem easy to read. I don't think we want both a title and a popover menu. Maybe @astridu can help?

I made this change pending @astridu's feedback:
Screen Shot 2019-06-19 at 11 51 29 AM

@astridu
Copy link

astridu commented Jun 19, 2019

Yes, the second example looks much better (just the 1 tooltip). Thank you!

@jcoyne
Copy link
Contributor

jcoyne commented Jun 20, 2019

I'm seeing this now. Since we have the tooltip, should we get rid of the <abbr/>?

Screen Shot 2019-06-20 at 9 24 45 AM

@jcoyne
Copy link
Contributor

jcoyne commented Jun 20, 2019

@michelleif @jermnelson what sort of commitments to we have to accessibility? I'm pretty sure the <abbr> with a title are useful for screen readers. I'm not sure the popover is useful for anyone not using a mouse.

@ndushay
Copy link
Contributor

ndushay commented Jun 20, 2019

would it be stupid to add a css styling that doesn't do the popover (because we display it another way?)

@jermnelson
Copy link
Contributor

@michelleif @jermnelson what sort of commitments to we have to accessibility? I'm pretty sure the <abbr> with a title are useful for screen readers. I'm not sure the popover is useful for anyone not using a mouse.

I would like us to be more accessible. We're running this elint plugin: https://www.npmjs.com/package/eslint-plugin-jsx-a11y with some rule exceptions.

@jcoyne
Copy link
Contributor

jcoyne commented Jun 20, 2019

So do we know if a screenreader can focus on a <sup> element that has a single child marked aria-hidden="true"?

@jgreben
Copy link
Contributor Author

jgreben commented Jun 20, 2019

I added a tabIndex to the sup to make it keyboard accessible, and role-tooltip for ARIA. Do you think this will address the accessibility issues?

@jcoyne
Copy link
Contributor

jcoyne commented Jun 20, 2019

Yes, that probably ought to do it.

@jcoyne jcoyne merged commit 3624206 into master Jun 20, 2019
Sinopia Work-Cycle One automation moved this from Needs Review to Done Jun 20, 2019
@jcoyne jcoyne deleted the fix-empty-tooltip branch June 20, 2019 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Tooltip is empty
5 participants