-
Notifications
You must be signed in to change notification settings - Fork 74
Adds tooltips to issues table #1053
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
base: new-main-page-UX
Are you sure you want to change the base?
Adds tooltips to issues table #1053
Conversation
- Tooltips can now be navigated using keyboard - Improved screen reader behavior
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.
By moving the clickable class to a cell () instead of the row (), the hover effect becomes kind of weird: only the left-most cell changes color, when the whole thing probably should.
| t={t} | ||
| /> | ||
| {/* ARIA live region for screen readers */} | ||
| <div aria-live="polite" style={{position: 'absolute', left: '-9999px', height: 0, width: 0, overflow: 'hidden'}}> |
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 love that you are using the correct accessible semantics: aria-live="polite" to make sure that the screen reader text interrupts, but not until the previous text is done, and role="dialog" below.
Is there a compelling reason to use a div with the role of dialog instead of a dialog element?
| aria-modal="true" | ||
| tabIndex={-1} | ||
| ref={popoverRef} | ||
| style={{ |
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.
For permanent styles, try to keep it in the CSS. I'm assuming that you did this because of the popover.x and popover.y values, which IS the right way to do this in React. Another approach could be to use CSS's attr() function. So give the element a dummy attribute, like data-pos-x={popover.x} and data-pos-y={popover.y}, then in the CSS, say left: attr(data-pos-x); top: attr(data-pos-y).
| </div> | ||
| <button | ||
| type="button" | ||
| style={{ |
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.
This styling should DEFINITELY be in CSS, since there isn't anything dynamic about it.
| > | ||
| <div | ||
| style={{ marginBottom: 8 }} | ||
| tabIndex={-1} |
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 have NO IDEA why, but each time I use the keyboard and the popup turns on, the focus goes straight to the text, even though it's explicitly set to tabIndex='-1'. Not sure how to approach that one.
| aria-haspopup="dialog" | ||
| aria-expanded={popover.open} | ||
| aria-controls={popover.open ? "issue-info-popover" : undefined} | ||
| style={{ |
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 this should be in a separate CSS file, and I'd like the following:
- The cursor should be a hand when over it.
- The icon should feel more clickable (maybe filled instead of transparent in the center).
- Consider changing the color/shade/shadow when hovering or focused on.
- Makes cursor a hand when hovering over help button - Adds a filled-in version of the help button - Adds an outline to help button to make it feel more responsive when hovered on - Moves CSS to its own file - Changes dialog type div to native dialog element - Changes focus to dialog box instead of text when dialog opened
|
- This icon now uses props for infill / icon colors - Uses standard --link-color and --link-hover colors
This PR adds a button next to the issues table of the reports to quickly see a barrier's description.
Fixes #1052