Skip to content

Comments

fix: add back button and centre icon on compare page#941

Merged
danielroe merged 3 commits intonpmx-dev:mainfrom
RYGRIT:fix/compare-page-style
Feb 4, 2026
Merged

fix: add back button and centre icon on compare page#941
danielroe merged 3 commits intonpmx-dev:mainfrom
RYGRIT:fix/compare-page-style

Conversation

@RYGRIT
Copy link
Contributor

@RYGRIT RYGRIT commented Feb 4, 2026

this pr:
related issues: #918

  1. added a back button.
  2. fixed the issue where the package close icon button was not vertically centered.
  3. fixed the issue where the tooltip did not display after being clicked on mobile devices.
  4. styling for inner padding in the empty state.
before after
image image

@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 2:35pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 4, 2026 2:35pm
npmx-lunaria Ignored Ignored Feb 4, 2026 2:35pm

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Updates three Compare-area components: FacetCard.vue now wraps the header info icon with a TooltipApp when a description exists; PackageSelector.vue adds flex alignment classes to the remove-package close icon span; compare.vue obtains a router instance via useRouter() for template navigation (e.g., back()) and adds horizontal padding to the empty-state layout.

Possibly related PRs

  • npmx-dev/npmx.dev PR 883 — applies the same code-level pattern of replacing inline info/title tooltips with TooltipApp wrappers around info icons.
  • npmx-dev/npmx.dev PR 936 — also adds a router instance and back-navigation usage in the compare page templates.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly relates to the changeset, detailing four specific improvements to the compare page.

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

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

  • 136.113.208.247/32 (new)
  • 34.170.211.100/32
  • 35.222.179.152/32

Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist.


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 (3)
app/components/Compare/FacetCard.vue (1)

78-84: Consider removing the redundant title attribute.

The span has both :title="description" and is wrapped in TooltipApp with :text="description". This may cause two tooltips to appear on desktop hover (the native browser tooltip from title and the custom tooltip from TooltipApp).

♻️ Suggested fix
       <TooltipApp v-if="description" :text="description" position="top">
         <span
           class="i-carbon:information w-3 h-3 text-fg-subtle"
-          :title="description"
           aria-hidden="true"
         />
       </TooltipApp>
app/pages/compare.vue (2)

67-68: Remove duplicate mb-4 from the heading.

The h1 has mb-4 but its parent div at line 66 also has mb-4. This creates unintended extra spacing. Since the parent container already provides the bottom margin, the mb-4 on the heading appears to be a leftover from before the layout change.

♻️ Suggested fix
-          <h1 class="font-mono text-3xl sm:text-4xl font-medium mb-4">
+          <h1 class="font-mono text-3xl sm:text-4xl font-medium">

72-72: Remove inline focus-visible utility class from the button.

The project uses global CSS for button focus-visible styling. Per coding guidelines, buttons should rely on the global rule in main.css rather than inline utility classes like focus-visible:outline-accent/70.

♻️ Suggested fix
-    class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+    class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"

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."

Comment on lines 70 to 77
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
>
<span class="i-carbon:arrow-left rtl-flip w-4 h-4" aria-hidden="true" />
<span class="hidden sm:inline">{{ $t('nav.back') }}</span>
</button>
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 4, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

router.back() may navigate users out of the site.

If a user navigates directly to the compare page (e.g., via a shared link), there's no history entry within the app. Calling router.back() would take them to their previous site or an empty tab, which is poor UX.

Consider using a fallback to a known route:

🛡️ Suggested fix with fallback
-            `@click`="router.back()"
+            `@click`="window.history.length > 1 ? router.back() : router.push('/')"

Alternatively, if there's a referrer check or you want to always navigate to a specific page (like the homepage), you could simplify to router.push('/').

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
@click="router.back()"
>
<span class="i-carbon:arrow-left rtl-flip w-4 h-4" aria-hidden="true" />
<span class="hidden sm:inline">{{ $t('nav.back') }}</span>
</button>
<button
type="button"
class="inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
`@click`="window.history.length > 1 ? router.back() : router.push('/')"
>
<span class="i-carbon:arrow-left rtl-flip w-4 h-4" aria-hidden="true" />
<span class="hidden sm:inline">{{ $t('nav.back') }}</span>
</button>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I didn't anticipate directly entering the compare page. Should we adopt the solution he mentioned?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RYGRIT Yes, I recommend adopting the suggested solution! Here's why:

The Problem:
When users arrive at the compare page via a direct link (shared URL, bookmark, etc.), calling router.back() would navigate them away from your site entirely—poor UX.

The Solution:

`@click`="window.history.length > 1 ? router.back() : router.push('/')"

This checks if there's navigation history before going back. If not, it safely redirects to the homepage.

Why it works:

  • ✅ Users who navigated within the app get expected back behavior
  • ✅ Users arriving from external sources stay on your site (go to homepage)
  • ✅ Simple, minimal code change
  • ✅ Follows common UX patterns

The fallback to / (homepage) makes sense as a safe default. Go ahead and implement it! 👍


🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@danielroe danielroe changed the title fix: compare page style issue fix: add back button and centre icon on compare page Feb 4, 2026
@danielroe danielroe enabled auto-merge February 4, 2026 14:27
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