Skip to content

fix: address CLS in downloads modal - #1619

Merged
graphieros merged 3 commits into
npmx-dev:mainfrom
graphieros:main
Feb 24, 2026
Merged

fix: address CLS in downloads modal#1619
graphieros merged 3 commits into
npmx-dev:mainfrom
graphieros:main

Conversation

@graphieros

@graphierosgraphieros commented Feb 24, 2026

Copy link
Copy Markdown
Member

Resolves#1342

  • Update the placeholder's aspect ratio
  • Set a min-h for the chart wrapper on desktop

Also fixes CLS introduced with the addition of the animation toggle:

  • Add a skeleton (visible when prefers-reduced-motion has no-preference)
  • Set a fixed height on the weekly downloads section of the side bar (shorter when prefers-reduced-motion is reduced, since the toggle is not displayed in this case)

@vercel

vercelBot commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
npmx.devReadyReadyPreview, CommentFeb 24, 2026 5:57am
2 Skipped Deployments
ProjectDeploymentActionsUpdated (UTC)
docs.npmx.devIgnoredIgnoredPreviewFeb 24, 2026 5:57am
npmx-lunariaIgnoredIgnoredFeb 24, 2026 5:57am

Request Review

@codecov

codecovBot commented Feb 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
app/components/Package/WeeklyDownloadStats.vue66.66%1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai

coderabbitaiBot commented Feb 24, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1fa4f2 and 27ae458.

📒 Files selected for processing (1)
  • app/components/Package/TrendsChart.vue
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/components/Package/TrendsChart.vue

📝 Walkthrough

Walkthrough

Updated two Vue components to adjust layout and skeleton placeholders only. In TrendsChart.vue the outer chart container now uses a dynamic class bound to isMobile and width to apply min-h-[260px] on mobile or min-h-[567px] otherwise. In WeeklyDownloadStats.vue the stats container height was increased (h-[110px], motion-safe:h-[140px]), an extra skeleton/placeholder element was added, and the modal chart aspect ratio changed from sm:aspect-[718/622.797] to sm:aspect-[718/647]. No data flow, state, or public API changes.

Possibly related PRs

Suggested reviewers

  • danielroe
  • serhalp
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check nameStatusExplanation
Description check✅ PassedThe PR description clearly relates to the changeset, detailing placeholder aspect ratio updates, minimum heights for desktop chart wrappers, and CLS fixes with skeleton additions and fixed heights.
Linked Issues check✅ PassedThe PR addresses all objectives from issue #1342: eliminates CLS by updating placeholder aspect ratios, setting minimum/fixed container heights, and implementing skeleton placeholders respecting prefers-reduced-motion.
Out of Scope Changes check✅ PassedAll changes are directly scoped to resolving issue #1342: responsive height bindings in TrendsChart.vue and container/skeleton adjustments in WeeklyDownloadStats.vue address only CLS and placeholder sizing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f4943b9 and 5519ca3.

📒 Files selected for processing (2)
  • app/components/Package/TrendsChart.vue
  • app/components/Package/WeeklyDownloadStats.vue

Comment threadapp/components/Package/TrendsChart.vue
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@graphieros
graphieros added this pull request to the merge queueFeb 24, 2026
Merged via the queue into npmx-dev:main with commit c0c9ff2Feb 24, 2026
16 checks passed

<template>
<div class="space-y-8">
<div class="space-y-8 h-[110px] motion-safe:h-[140px]">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @graphieros, I've noticed that this change causes a large blank area after the "Weekly Downloads" section collapses.

Image

I'm considering moving the height limit (min-h-[110px] motion-safe:min-h-[140px]) into the inner container of CollapsibleSection, but I'm unsure if this complies with the CLS specification.

If it does, please let me know, and I'd be happy to submit a follow-up PR!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please :)

alex-key pushed a commit to alex-key/npmx.dev that referenced this pull request Feb 25, 2026
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLS when switching facets in trends modal

3 participants

@graphieros@43081j@RYGRIT