feat: add bar chart view to compare page#1974
feat: add bar chart view to compare page#1974graphieros wants to merge 22 commits intonpmx-dev:mainfrom
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
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
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 Compare/FacetBarChart.vue that renders horizontal facet comparison bar charts with export callbacks (CSV/PNG/SVG) and alt-text copy support. Introduces new chart utilities and helpers in app/utils/charts.ts (loadFile, sanitise, insertLineBreaks, applyEllipsis, and facet-bar alt-text functions). Propagates a new boolean Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/compare.vue (1)
285-365:⚠️ Potential issue | 🟠 MajorThis view switch is not keyboard-operable as a tabs control.
The inactive tab is removed from the tab order with
tabindex="-1", but there is no arrow-key roving focus logic, so keyboard users cannot reach the other view. The panels also never expose the IDs referenced byaria-controls, so the tab/panel relationship is incomplete for assistive tech. Either implement the full tabs pattern or keep these as ordinary toggle buttons.
🧹 Nitpick comments (4)
app/composables/useFacetSelection.ts (1)
12-16: Keepchartablein shared facet metadata.
chartableis behavioural metadata rather than translated copy. Keeping it insidefacetLabelsties chart support to i18n and makes this composable the only source of truth. I’d move it intoFacetInfo/FACET_INFOand leavefacetLabelsresponsible only forlabelanddescription.Also applies to: 27-104
app/pages/compare.vue (1)
297-297: Use the shared button focus treatment here instead.These tab buttons are native
<button>elements, so the inlinefocus-visible:outline-accent/70class just duplicates and diverges from the project-wide focus ring.Based on learnings, focus-visible styling for buttons and selects is applied globally in
app/assets/main.css, and individual buttons should not add inlinefocus-visible:outline-accent/70utilities.Also applies to: 316-316
app/utils/charts.ts (1)
614-618: Remove the stray console log from the alt-text helper.
createAltTextForCompareFacetBarChart()sits on a user-facing menu action, so this will log every alt-text copy in production and pollute tests/devtools for no value.🧹 Proposed fix
if (!dataset) return '' const { facet, description, $t } = config - - console.log(config) const packages = dataset.map(d => d.name).join(', ')test/unit/app/utils/charts.spec.ts (1)
1-23: Please cover the compare-facet alt-text helpers as well.This file adds good coverage for the new formatting utilities, but
createAltTextForCompareFacetBarChart()andcopyAltTextForCompareFacetBarChart()are the new public accessibility path behindFacetBarChartand are still untested here. A small suite mirroring the trend/version chart tests would catch translation-param and copy-forwarding regressions.As per coding guidelines,
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest.Also applies to: 1244-1455
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb495d21-75d7-4d37-bb84-c076617d44fb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
app/components/Compare/FacetBarChart.vueapp/components/Package/TrendsChart.vueapp/components/Package/VersionDistribution.vueapp/composables/useFacetSelection.tsapp/pages/compare.vueapp/utils/charts.tsi18n/locales/en.jsoni18n/schema.jsonpackage.jsontest/nuxt/a11y.spec.tstest/unit/app/utils/charts.spec.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Compare/FacetBarChart.vue (1)
238-246: Fallback skeleton usespkgas key—confirm uniqueness.Using the package name directly as
:key="pkg"is fine if package names are guaranteed unique within the comparison. If duplicates are ever possible, consider${pkg}-${index}to ensure stable keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d299d82-5cae-43ec-aa88-35ebad1cb04d
📒 Files selected for processing (1)
app/components/Compare/FacetBarChart.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Compare/FacetBarChart.vue (2)
14-14: Consider using a standard CSS import for critical styles.The dynamic
import()at module level for stylesheets works but is unconventional. The discarded promise means styles load asynchronously, which could cause a brief flash of unstyled content on slower connections. A regular import ensures styles are bundled and available synchronously.-import('vue-data-ui/style.css') +import 'vue-data-ui/style.css'If lazy-loading is intentional for bundle optimisation, consider documenting the rationale.
66-72: Add a clarifying comment for the watcher logic.The early return when lengths differ is correct (the chart re-renders naturally when dataset size changes), but the intent isn't immediately obvious. A brief comment would help future maintainers.
watch( () => props.packages, (newP, oldP) => { + // When length changes, the chart re-renders via dataset change. + // When packages are swapped (same length), force re-render via key. if (newP.length !== oldP.length) return chartKey.value += 1 }, )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b3fb29c-1836-4f5f-aa80-5ffb3f95d3d4
📒 Files selected for processing (1)
app/components/Compare/FacetBarChart.vue
|
Visually, I really like everything except the text size. The package name is initially 16px, but since the container is scaled down, it's now ~9px. I don't know if we can safely increase it, but I think it would be helpful to add a title to the entire line, in the format @knowler wdyt? |
|
@alexdln I enabled the tooltip, and also increased the font size for names slightly, which should solve the problem |
2 participants
Some users find it easier to read information from tables, others from charts.
This offers the possibility to switch from one view to the other to compare chartable facets.
The charts for downloads history remains independent, and visible with both views.
Desktop:
Mobile:
