search

mozilla / perfcompare

Improved Performance Comparison Tool
Mozilla Public License 2.0
39 stars 93 forks source link

PCF-361 Add aria roles for tables / rows / rowgroup etc #591

Closed esanuandra closed 9 months ago

esanuandra commented 9 months ago

This is a follow-up PR to address https://github.com/mozilla/perfcompare/pull/580#pullrequestreview-1770187205

netlify[bot] commented 9 months ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit b5c844be77fe967b6fd30169c9bcad71d86c6b42
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/65a8d85c3d830e00085017d2
Deploy Preview https://deploy-preview-591--mozilla-perfcompare.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (3d0654a) 99.34% compared to head (b5c844b) 99.34%. Report is 1 commits behind head on beta.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## beta #591 +/- ## ======================================= Coverage 99.34% 99.34% ======================================= Files 62 62 Lines 1366 1366 Branches 189 189 ======================================= Hits 1357 1357 Misses 9 9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

julienw commented 9 months ago

Looking at the accessibility panel, the structure looks good to me! I'm just not sure about the expanded row, if that should be a separate row. Ideally it should be inside the "parent" row, but not 100% sure how that should look in terms of nesting and roles. I think this could still go in with this structure and we can improve this part specifically later.

julienw commented 9 months ago

I thought about that more over the night, now I think the expanded row shouldn't have the role=row, just make it a normal div. Later on we can work with aria-expanded and aria-owns but let's not focus on that now.