-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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(chart table in dashboard): improve screen reading of table #26453
fix(chart table in dashboard): improve screen reading of table #26453
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.
@ncar285 looks like there are some linting errors that are causing the tests to fail. I can comment on some in the PR, but you can also run |
superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks @ncar285! Looks really good. Just some small suggestions for linting and cleanup.
superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx
Show resolved
Hide resolved
…ks/useSticky.tsx Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
…ks/useSticky.tsx Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #26453 +/- ##
==========================================
- Coverage 69.18% 69.16% -0.03%
==========================================
Files 1945 1948 +3
Lines 75948 76037 +89
Branches 8460 8493 +33
==========================================
+ Hits 52546 52591 +45
- Misses 21217 21266 +49
+ Partials 2185 2180 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/testenv up |
@yousoph Ephemeral environment spinning up at http://54.184.85.44:8080. Credentials are |
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 looks great @ncar285. I'll leave the PR open for testing, and then we'll merge. Thank you!
Ephemeral environment shutdown and build artifacts deleted. |
…e#26453) Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
@ncar285 @eschutho I think after this PR the Totals are not sticky anymore 🙁 Screen.Recording.2024-01-31.at.17.01.57.mov |
apache#26453)" This reverts commit 71a950f.
Thanks for catching @kgabryje! We'll take a look. 🙏 |
Yes thanks for pointing that out!
…On Fri, 2 Feb 2024 at 10:29, Elizabeth Thompson ***@***.***> wrote:
Thanks for catching @kgabryje <https://github.com/kgabryje>! We'll take a
look. 🙏
—
Reply to this email directly, view it on GitHub
<#26453 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A73OXZ2RN2FQA4OHHWRBPGTYRQCKLAVCNFSM6AAAAABBVVZRRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRSGI3TQMJRGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…ve screen reading of table (apache#26453)" (apache#26963) (cherry picked from commit e4eae9a)
…e#26453) Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
…ve screen reading of table (apache#26453)" (apache#26963)
…e#26453) Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
…ve screen reading of table (apache#26453)" (apache#26963)
SUMMARY
Enhancing Accessibility with Unified Table Structure and Sticky Header
This PR addresses accessibility concerns by merging the header and body of our tables into a single unified structure. This change significantly improves screen reader compatibility, ensuring a more inclusive user experience. Previously, the header and body of a chart were seperate tables. Visually this worked but caused issues with screen reader functionality.
Key Change:
Technical Challenge:
A notable challenge was maintaining a scrollable body while keeping the header fixed. Setting overflow directly on
tbody
proved unfeasible. Wrappingtbody
in a div with an overflow disrupted the column grouping, as the table would then interpret the entiretbody
as a single column. The implemented solution achieves the desired layout without compromising the table's integrity or accessibility.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION