-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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(accessibility): add tabbing to chart menu in dashboard #26138
Conversation
65ad28e
to
729d07c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #26138 +/- ##
==========================================
- Coverage 69.69% 69.68% -0.01%
==========================================
Files 1908 1908
Lines 74530 74612 +82
Branches 8309 8333 +24
==========================================
+ Hits 51942 51993 +51
- Misses 20535 20565 +30
- Partials 2053 2054 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.213.200.158:8080. Credentials are |
When using keyboard navigation to tab through on a dashboard, the highlight on the chart title in dashboards doesn't seem to be appearing for me |
85ca7b6
to
dfde2cf
Compare
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.202.14.64:8080. Credentials are |
7b1f0dd
to
3da50cc
Compare
/testenv up |
@eschutho Ephemeral environment spinning up at http://52.35.108.27:8080. Credentials are |
3da50cc
to
dbb08a7
Compare
8d1a78b
to
98b9161
Compare
/testenv up |
@eschutho Ephemeral environment spinning up at http://35.87.72.46:8080. Credentials are |
I think this looks good! Thanks @eschutho 🙏 |
c1543e7
to
167dda6
Compare
c225633
to
a073e57
Compare
a073e57
to
81efffb
Compare
@@ -170,25 +182,91 @@ const dropdownIconsStyles = css` | |||
} | |||
`; | |||
|
|||
export const handleDropdownNavigation = ( |
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 know that this PR is focused on dashboard interactions, but maybe (in future iterations if it's too complex?) we should add this functionality to the main Dropdown component, so that the keyboard navigation works in all dropdowns?
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.
Yeah, good idea!
When I'm navigating the dropdown with arrows, the whole page scrolls with every click 🙁 And it looks like I can't navigate to Submenus Screen.Recording.2024-02-27.at.17.49.28.mov |
Yeah, I didn't add the submenus yet. That's for a separate PR. |
ToggleFullscreen = 'toggle_fullscreen', | ||
ManageEmbedded = 'manage_embedded', | ||
ManageEmailReports = 'manage_email_reports', | ||
} |
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 this file just for typescript types or .. more?
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.
What do you mean for "more"?
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 thought maybe this file was only to be used for typescript types rather than runtime enums.
Ephemeral environment shutdown and build artifacts deleted. |
…6138) Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]>
@@ -771,7 +771,7 @@ describe('Dashboard edit', () => { | |||
visitEdit(); | |||
}); | |||
|
|||
it('should save', () => { | |||
it.skip('should save', () => { |
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.
@eschutho I'm not sure the fate of these tests... should we scrap them, or try to rekindle them?
Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]> (cherry picked from commit 34b1db2)
…6138) Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]>
…6138) Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]>
Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]> (cherry picked from commit 34b1db2)
…6138) Co-authored-by: geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]>
Hello, I'm unsure why the following change was made which introduce superset/superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx Line 71 in 34b1db2
|
SUMMARY
This change adds the ability to tab through elements on a dashboard, the title, charts, and into the chart controls, and execute each of them with a keyboard.
Screen.Recording.2024-02-06.at.5.56.19.PM.mov
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION