fix: improve chart and tab accessibility - #422
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR adds variant-aware alt text for chart images and lightbox content. It also adds ChangesAccessibility updates
Merge Risk: ⚪ Minimal · up to This PR makes localized chart and tab accessibility improvements while preserving existing dashboard behavior and data. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
255b90b to
f6c2084
Compare
f6c2084 to
6befaff
Compare
📊 Dashboard previewThe dashboard was built for this PR. ➡️ Download
|
exploreriii
left a comment
There was a problem hiding this comment.
Hi @5affron
Please check for code quality please, as the code base grows we will get growign pains with maintainability issues
|
@exploreriii Thanks for the review. I addressed this by extracting the shared activeVariant and chartAlt helpers so the chart and lightbox use the same variant handling without duplicating the logic. I also cleaned up the test assertions to avoid duplicating the pressed-state checks. Full tests, build, and lint pass with no lint errors. |
|
Hi @exploreriii, @mgarbs, @MonaaEid — I’ve gone through the comments and made the requested changes. I removed the unrelated changes, cleaned up the repeated logic by moving it into helpers, and simplified the test that was checking the same thing twice. I also ran the checks locally — Ruff, formatting, oxlint, the build, and all 62 tests are passing. The PR is now updated with only the intended changes. If there’s anything else you think needs to be taken care of, please let me know here and I’ll take a look. Thanks for the feedback! |
phillip-nyinomujuni
left a comment
There was a problem hiding this comment.
Author already addressed the maintainability feedback from the last round (helper extraction, deduped test assertions) confirmed via diff, not re-raising that. The alt text fix and aria-pressed addition both look correct and are covered by tests. Ruff/lint/build reported clean per the author's comment. No new blocking issues from me — approving.
|
Please check GPG key signing @5affron |
Signed-off-by: 5affron <piyushrajxsaffron@gmail.com>
|
@5affron rebase please to avoid the conflict, else we will probably append a commit ASAP to do so |
The dashboard rework (hiero-hackers#490) replaced the chart images, lightbox and TabBar this branch changed; conflicts resolve to main's versions. Signed-off-by: Ntege Daniel <danientege785@gmail.com>
A single-view chart's only variant label is its own title, so its figure was announced as "Activity heatmap — Activity heatmap", and its loading and error messages repeated the title the same way. Name the view with its title alone when the label adds nothing, and keep "<title> — <variant>" for charts with tabs. Refs hiero-hackers#326 Signed-off-by: Ntege Daniel <danientege785@gmail.com>
Picks up the codeql-action fix (hiero-hackers#505) so CodeQL runs on this PR. Signed-off-by: Ntege Daniel <danientege785@gmail.com>
Description
Improve chart and tab accessibility in the analytics dashboard.
Related issue(s)
Fixes #326
Testing
uv run pytestsuccessfully.hiero-ledgerandhiero-hackers.Checklist
/assignbefore startinguv run pytestand the relevant checks pass locallygit commit -S -s