feat: compare download charts with sparklines#2273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
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:
📝 WalkthroughWalkthroughAdds a new Vue 3 component app/components/Chart/SplitSparkline.vue that renders a grid of per-unit sparklines from a provided dataset and dates, pairing missing values with 0, providing per-series colours and dash indices, i18n-driven accessibility text, light/dark mode styling, hover/selection handling and custom hint/skeleton slots, and wraps each chart in ClientOnly. Updates app/components/Package/TrendsChart.vue to add an accessible two-option tab UI and a reactive isSparklineLayout toggle to switch between the combined VueUiXy chart and the split-sparkline layout. Adds i18n keys/schema entries, accessibility tests, and bumps vue-data-ui to 3.16.3. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/nuxt/a11y.spec.ts (1)
946-999: Please add aPackageTrendsChartsplit-view a11y case as well.These assertions only mount
ChartSplitSparklinein isolation. The user-facing behaviour added by the PR also includes the toggle and conditional render path inPackageTrendsChart, so wiring bugs there can still slip through without a multi-package integration case that switches to split view.app/components/Package/TrendsChart.vue (1)
1649-1649: Keep the new buttons on the shared focus-visible rule.These are plain
<button>elements, so the inlinefocus-visible:outline-accent/70will drift from the project's global button focus styling. Please rely on the shared rule instead of reintroducing per-element focus utilities here.Based on learnings,
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }is applied globally inapp/assets/main.css.Also applies to: 1668-1668
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae693dfe-6e50-4022-b706-c16d24fb6a81
📒 Files selected for processing (6)
app/components/Chart/SplitSparkline.vueapp/components/Package/TrendsChart.vuei18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsontest/nuxt/a11y.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/Package/TrendsChart.vue (1)
1645-1649:⚠️ Potential issue | 🟠 MajorTab keyboard semantics are still broken
For
role="tab", the selected tab should be keyboard-focusable and arrow keys should switch tabs. Here, Line 1648 and Line 1667 invert the expectedtabindexstate, and there is no arrow-key handling, so keyboard users can get stuck.Suggested fix (proper tabs behaviour)
- :tabindex="isSparklineLayout ? 0 : -1" + :tabindex="isSparklineLayout ? -1 : 0" + `@keydown.right.prevent`="isSparklineLayout = true" + `@keydown.left.prevent`="isSparklineLayout = false" + `@keydown.home.prevent`="isSparklineLayout = false" + `@keydown.end.prevent`="isSparklineLayout = true" @@ - :tabindex="!isSparklineLayout ? 0 : -1" + :tabindex="!isSparklineLayout ? -1 : 0" + `@keydown.right.prevent`="isSparklineLayout = true" + `@keydown.left.prevent`="isSparklineLayout = false" + `@keydown.home.prevent`="isSparklineLayout = false" + `@keydown.end.prevent`="isSparklineLayout = true"Also applies to: 1664-1668
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1649-1649: Remove per-buttonfocus-visibleutility classesThese button classes should rely on the project’s global
button:focus-visiblestyling instead of inlinefocus-visible:*utilities.Suggested class cleanup
- class="... transition-colors duration-150 focus-visible:outline-accent/70" + class="... transition-colors duration-150"Based on learnings: focus-visible styling for button/select elements is globally defined in
app/assets/main.css; per-element focus-visible utilities should be avoided.Also applies to: 1668-1668
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30ac90a1-5f0d-4c6a-8cf5-16f3dc351218
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da1bb4f0-f563-45cd-83a7-50356a4ef1af
📒 Files selected for processing (4)
app/components/Package/TrendsChart.vuei18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.json
✅ Files skipped from review due to trivial changes (2)
- i18n/locales/en.json
- i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/locales/fr-FR.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…-split-view-option-to-display-series-in-individual-sparklines
|
All previous errors are resolved now 🚀 |
These are great improvements, thanks :) |
MatteoGabriele
left a comment
There was a problem hiding this comment.
Awesome! Thank you for checking those changes ❤️
Resolves #2270
This implements an additional view for the download charts of the compare page, to address a11y issues related to color vision deficiency, and display each series on a separate sparkline chart. @Adebesin-Cell also co-authored this PR with:
A toggle allows to switch from the 'Combined view' to the 'Split view'.
Hovering a sparkline chart syncs the selected index on all charts to ease comparison:
Long package names are ellipsed to avoid any Y offset that a wrap would cause.
Other:
Notes: