Skip to content

Conversation

@wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Feb 4, 2026

Skeleton was misaligned due to:

  • Skeleton being positioned differently (quirky because of baseline alignment of parent flex element)
  • Skeleton being too small
  • MetricsBadged introducing extra margins unexpectedly
  • Missing "white skeleton" for Like button

Before:

Nagranie.z.ekranu.2026-02-4.o.12.31.24.mov

It's actually even worse than that because also the right toolbar with docs, code and compare may jump to the next line because of it.

After:

Nagranie.z.ekranu.2026-02-4.o.12.30.59.mov

Copilot AI review requested due to automatic review settings February 4, 2026 11:35
@vercel
Copy link

vercel bot commented Feb 4, 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 4, 2026 0:53am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 4, 2026 0:53am
npmx-lunaria Ignored Ignored Feb 4, 2026 0:53am

Request Review

</ClientOnly>
<template #fallback>
<div
class="flex items-center gap-1.5 list-none m-0 p-0 relative top-[5px] self-baseline ms-1 sm:ms-2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The relative top-[5px] sucks a bit, but gets the job done. Honestly, I have no better idea how to fix it while keeping baseline alignment.

Copy link
Member

Choose a reason for hiding this comment

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

what about having the text visible so it's just the icon that loads? we could even replace it with a spinner to give better feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably a good idea, but if I may have a say, I'd do it as a next iterative improvement. It's likely not going to be such an easy fix as this was.

Copy link
Member

Choose a reason for hiding this comment

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

pushed a first stab - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet!

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the alignment of skeleton loading placeholders for the MetricsBadges component by adjusting the positioning, sizing, and flex layout structure.

Changes:

  • Restructured the ClientOnly wrapper to be inside the flex container instead of wrapping it
  • Added contents class to list items in MetricsBadges to eliminate list item boxes and improve flex layout behavior
  • Changed badge elements from inline-flex to flex for better integration with the parent flex layout

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
app/pages/package/[...package].vue Moved ClientOnly wrapper inside flex container and adjusted skeleton fallback styling with positioning and sizing changes
app/components/Package/MetricsBadges.vue Added contents class to list items and changed badge display from inline-flex to flex

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Destructures status from usePackageAnalysis and adds isLoading in Package/MetricsBadges.vue. Badges (ESM, CJS, Types) now render loading-aware wrappers: subdued styling and a spinner while isLoading is true; icons and tooltips reflect availability (hasTypes, hasEsm, hasCjs) only when not loading. The root <ul> no longer has a top-level v-if; individual items manage visibility. In app/pages/package/[...package].vue the ClientOnly wrapper was moved to wrap the inner metrics row, the PackageMetricsBadges skeleton expanded, and the component now receives explicit version and is-binary props.

Possibly related PRs

  • fix(ui): align like button #926: Adjusts the same package page DOM/layout around PackageMetricsBadges (ClientOnly wrapper and flex container restructuring), overlapping the area modified here.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly relates to the changeset, identifying specific UI alignment issues with MetricsBadges skeleton and proposing visual improvements with before/after screenshots.

✏️ 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: 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/Package/MetricsBadges.vue (1)

57-71: ⚠️ Potential issue | 🟡 Minor

Potential list semantics loss with display: contents on <li>

display: contents can cause some assistive tech to ignore list items. If you want to keep list semantics, consider adding role="listitem" to the badge wrapper (or avoid contents).

🔧 Possible low-impact fix
-    <li v-if="!props.isBinary" class="contents">
-      <TooltipApp :text="typesTooltip">
+    <li v-if="!props.isBinary" class="contents">
+      <TooltipApp :text="typesTooltip" role="listitem">
         <component
           :is="typesHref ? NuxtLink : 'span'"
           :to="typesHref"
           :tabindex="!typesHref ? 0 : undefined"
           class="flex items-center gap-1 px-1.5 py-0.5 font-mono text-xs rounded transition-colors duration-200 focus-visible:(outline-2 outline-accent)"

-    <li class="contents">
-      <TooltipApp :text="hasEsm ? $t('package.metrics.esm') : $t('package.metrics.no_esm')">
+    <li class="contents">
+      <TooltipApp :text="hasEsm ? $t('package.metrics.esm') : $t('package.metrics.no_esm')" role="listitem">
         <span
           tabindex="0"
           class="flex items-center gap-1 px-1.5 py-0.5 font-mono text-xs rounded transition-colors duration-200 focus-visible:(outline-2 outline-accent)"

-    <li v-if="hasCjs" class="contents">
-      <TooltipApp :text="$t('package.metrics.cjs')">
+    <li v-if="hasCjs" class="contents">
+      <TooltipApp :text="$t('package.metrics.cjs')" role="listitem">
         <span
           tabindex="0"
           class="flex items-center gap-1 px-1.5 py-0.5 font-mono text-xs text-fg-muted bg-bg-muted border border-border rounded transition-colors duration-200 focus-visible:(outline-2 outline-accent)"

Also applies to: 84-93, 106-111

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

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

Files with missing lines Patch % Lines
app/components/Package/MetricsBadges.vue 94.11% 1 Missing ⚠️
app/pages/package/[...package].vue 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

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

<SkeletonBlock class="w-13 h-5.5 rounded" />
<SkeletonBlock class="w-13 h-5.5 rounded" />
<div class="w-13 h5.5" />
<SkeletonBlock class="w-13 h-5.5 rounded bg-bg-subtle" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally used a white div as the like button is itself white, but I'm okay with this.

@danielroe danielroe added this pull request to the merge queue Feb 4, 2026
Merged via the queue into npmx-dev:main with commit aecacb2 Feb 4, 2026
16 checks passed
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