Skip to content

Comments

fix: stabilise modal chart render after transition#1034

Merged
danielroe merged 10 commits intonpmx-dev:mainfrom
graphieros:main
Feb 6, 2026
Merged

fix: stabilise modal chart render after transition#1034
danielroe merged 10 commits intonpmx-dev:mainfrom
graphieros:main

Conversation

@graphieros
Copy link
Contributor

@graphieros graphieros commented Feb 5, 2026

Ever since the chart modal was placed inside a dialog element, the chart component started showing flaky behavior:

  • on mobile: almost never showed the chart's minimap
  • on desktop: almost half of the time

Fix:

  • only render the modal's content after its transition is done
  • an empty placeholder, with the same aspect ratio of the content, avoids CLS while waiting for the transition to end

This adds a subtle delay before the content is displayed inside the chart modal, but for now it is better than a flaky behavior.

@vercel
Copy link

vercel bot commented Feb 5, 2026

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

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 6, 2026 2:06pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 6, 2026 2:06pm
npmx-lunaria Ignored Ignored Feb 6, 2026 2:06pm

Request Review

@codecov
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

❌ Patch coverage is 5.26316% with 18 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/components/Package/WeeklyDownloadStats.vue 0.00% 11 Missing ⚠️
app/components/Modal.client.vue 12.50% 3 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The Modal component declares an emitted transitioned event via defineEmits and emits it from an onDialogTransitionEnd handler attached to the dialog element's transitionend listener. WeeklyDownloadStats adds hasChartModalTransitioned state and handleModalClose and handleModalTransitioned handlers; openChartModal resets the flag before opening. The modal now emits close and transitioned; chart content is mounted only after transitioned fires, with a skeleton placeholder kept until the transition completes. A scoped CSS opacity transition was added.

🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description is directly related to the changeset, clearly explaining the flaky modal chart rendering issue and the implemented fix.

✏️ 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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

🧹 Nitpick comments (1)
app/components/Package/WeeklyDownloadStats.vue (1)

15-21: Double nextTick may be overly cautious.

The double await nextTick() pattern is sometimes needed when multiple reactive updates need to flush, but it can be fragile and obscure. Consider whether a single nextTick() suffices, or document why two are required here.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/Modal.client.vue (1)

45-49: ⚠️ Potential issue | 🟠 Major

Address closedby="any" attribute browser compatibility gap.

The closedby="any" attribute lacks support in Safari (both macOS and iOS), which remains an open WebKit issue. Chrome and Firefox added support in 2025, but Edge support is shipping in March 2026. For Safari users, the attribute will be silently ignored, resulting in loss of the dismiss-on-outside-click behaviour. Consider implementing a fallback mechanism or feature detection for unsupported browsers, or document this as a known limitation for Safari users.

🧹 Nitpick comments (1)
app/components/Modal.client.vue (1)

56-63: Remove inline focus-visible utility from button.

The close button uses an inline focus-visible:outline-accent/70 class, but focus-visible styling for buttons is handled globally in main.css. Rely on the global rule for consistency.

Proposed fix
       <button
         type="button"
-        class="text-fg-subtle w-5 h-5 hover:text-fg transition-colors duration-200 focus-visible:outline-accent/70 rounded"
+        class="text-fg-subtle w-5 h-5 hover:text-fg transition-colors duration-200 rounded"
         :aria-label="$t('common.close')"
         `@click`="handleModalClose"
       >

Based on learnings: "In the npmx.dev project, focus-visible styling for button and select elements is implemented globally in app/assets/main.css… Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements."

@danielroe danielroe added this pull request to the merge queue Feb 6, 2026
Merged via the queue into npmx-dev:main with commit 32d1d0a Feb 6, 2026
16 of 17 checks passed
whitep4nth3r pushed a commit that referenced this pull request Feb 7, 2026
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free to 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.

2 participants