-
-
Notifications
You must be signed in to change notification settings - Fork 237
feat: add permalink to downloads modal #1299
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
feat: add permalink to downloads modal #1299
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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:
📝 WalkthroughWalkthroughThis pull request introduces a centralised type system for chart data structures and implements route-driven modal state management. A new 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
This introduces a regression, the data fetched does not correspond to the expected range:
Also, the refresh button next to the date inputs, that shows when changing the date or granularity, stays visible when used, yet it should only appear when the selection or granularity is different than the default 52 weeks |
|
@graphieros, would you mind giving this another look? I think the graph is now working as expected, and the reset button only shows up when the dates change and disappears when pressed. wdyt? 🙏 |
|
Looks good to me :) |
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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/components/Package/TrendsChart.vue (1)
1535-1543: Remove inlinefocus-visibleutility class from the reset button.The project applies
focus-visiblestyling for buttons globally viamain.css. Inline utilities likefocus-visible:outline-accent/70should be avoided for consistency. Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."♻️ Proposed fix
class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border focus-visible:outline-accent/70 sm:mb-0" + class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border sm:mb-0"app/components/Package/WeeklyDownloadStats.vue (1)
253-262: Remove inlinefocus-visibleutility class from the button.The project applies
focus-visiblestyling for buttons globally viamain.css. Inline utilities likefocus-visible:outline-accent/70should be avoided for consistency. Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."♻️ Proposed fix
- class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded" + class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded"
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/TrendsChart.vue (1)
322-360:⚠️ Potential issue | 🟠 MajorPrevent permalink mode from activating when the prop is unset.
Passing
permanent: props.permalinkmeansundefinedfalls back totrueinsideusePermalink, so non-permalink contexts will still sync to the URL. Default the prop tofalse(or coalesce) before passing it into the composable.✅ Suggested fix (single source of truth)
+const permalinkEnabled = props.permalink ?? false + -const selectedGranularity = usePermalink<ChartTimeGranularity>('granularity', DEFAULT_GRANULARITY, { - permanent: props.permalink, -}) +const selectedGranularity = usePermalink<ChartTimeGranularity>('granularity', DEFAULT_GRANULARITY, { + permanent: permalinkEnabled, +}) -const startDate = usePermalink<string>('start', '', { - permanent: props.permalink, -}) +const startDate = usePermalink<string>('start', '', { + permanent: permalinkEnabled, +}) -const endDate = usePermalink<string>('end', '', { - permanent: props.permalink, -}) +const endDate = usePermalink<string>('end', '', { + permanent: permalinkEnabled, +}) -const selectedMetric = usePermalink<MetricId>('facet', DEFAULT_METRIC_ID, { - permanent: props.permalink, -}) +const selectedMetric = usePermalink<MetricId>('facet', DEFAULT_METRIC_ID, { + permanent: permalinkEnabled, +})Also applies to: 586-588
|
Now I want our permalinks to produce opengraph images of the charts 😈 |
Adds query parameters to support the downloads modal permalink.
When present, the modal opens on page load and persists across refreshes.
Included parameters: granularity, facet, start date, and end date.
link to a modal
Screen.Recording.2026-02-09.at.22.19.01.mov
closes #516