feat(ui): add e18e module replacement recommendations to /compare page#801
feat(ui): add e18e module replacement recommendations to /compare page#801
/compare page#801Conversation
and add somewhat of an Easter egg for manual "no dependency" comparisons when typing certain trigger queries like "none" or "no dep" or "diy".
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Absolutely love the easter egg! |
serhalp
changed the title
/compare page/compare page
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughIntroduces explicit “no dependency” support and per-column replacement metadata across the compare feature. Replaces numeric/grid header props with a typed ComparisonGridColumn[] and add showNoDependency flag in ComparisonGrid.vue. Adds ReplacementSuggestion component and useCompareReplacements composable to fetch and classify ModuleReplacement data. Extends usePackageComparison with NO_DEPENDENCY_ID and synthetic no-dependency rows. Updates pages/compare.vue to build gridColumns (header + replacement), expose suggestion UI, and keep the no-dep column last. Adds i18n keys, tooltip interactive/content support, and associated unit and e2e tests. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/components/Compare/PackageSelector.vue (1)
58-63: Consider: Enter key doesn't select visible no-dependency option.When the user types a trigger phrase (e.g., "no dep") and the no-dependency option is visible, pressing Enter adds the literal text as a package name rather than selecting the easter egg option.
This may be intentional for the easter egg discovery, but keyboard-only users might find it confusing when the option is visible but Enter doesn't select it. Consider enhancing keyboard navigation if this becomes a UX concern.
app/components/Compare/ComparisonGrid.vue (1)
117-131: Consider addingaria-describedbyfor improved screen reader support.The tooltip triggers correctly use
@focus/@blurfor keyboard accessibility. For enhanced screen reader support, consider linking the trigger to its tooltip viaaria-describedby:♿ Accessibility enhancement
<button v-if="specialColumns?.[index]" type="button" class="inline-flex items-center gap-1.5 cursor-help bg-transparent border-0 p-0" :aria-label="$t('compare.no_dependency.label')" + :aria-describedby="activeTooltip?.type === 'nodep' ? 'nodep-tooltip' : undefined" `@mouseenter`="showNoDep" `@mouseleave`="hideTooltip" `@focus`="showNoDep" `@blur`="hideTooltip" >And add matching
idto the tooltip:<div v-if="activeTooltip?.type === 'nodep'" + id="nodep-tooltip" role="tooltip" ... >app/pages/compare.vue (1)
64-73: Consider defensive access forpackages.value[i].Whilst the fallback
?? ''handles the case wherepackages.value[i]is undefined, this relies onpackagesDataandpackagesarrays staying in sync. The current implementation should work correctly as both derive from the same reactive source, but adding an explicit bounds check would make the intent clearer.🔧 Optional: More explicit bounds check
return packagesData.value.map((p, i) => p ? p.package.version ? `${p.package.name}@${p.package.version}` : p.package.name - : (packages.value[i] ?? ''), + : (i < packages.value.length ? packages.value[i] : '') ?? '', )test/nuxt/composables/use-compare-replacements.spec.ts (1)
6-39: Helper function captures composable state effectively.The wrapper component pattern correctly uses
watchEffectto synchronise the composable's reactive outputs to external refs for assertion. This is a valid approach for testing composables that rely on Vue's reactivity system.However, the mounted component is never unmounted, which could lead to memory leaks or test pollution in larger test suites.
🧹 Return wrapper for cleanup
async function useCompareReplacementsInComponent(packageNames: string[]) { const capturedNoDepSuggestions = ref<ReplacementSuggestion[]>([]) const capturedInfoSuggestions = ref<ReplacementSuggestion[]>([]) const capturedLoading = ref(false) const capturedReplacements = ref(new Map<string, ModuleReplacement | null>()) const WrapperComponent = defineComponent({ setup() { const { noDepSuggestions, infoSuggestions, loading, replacements } = useCompareReplacements(packageNames) watchEffect(() => { capturedNoDepSuggestions.value = [...noDepSuggestions.value] capturedInfoSuggestions.value = [...infoSuggestions.value] capturedLoading.value = loading.value capturedReplacements.value = new Map(replacements.value) }) return () => h('div') }, }) - await mountSuspended(WrapperComponent) + const wrapper = await mountSuspended(WrapperComponent) return { noDepSuggestions: capturedNoDepSuggestions, infoSuggestions: capturedInfoSuggestions, loading: capturedLoading, replacements: capturedReplacements, + unmount: () => wrapper.unmount(), } }Then call
unmount()in tests or add toafterEach.
- replace custom tooltip with reuse of TooltipApp - add ability to TooltipApp to accept an interactive slot, so that we can render a link inside the tooltip - refactor various bits for simplicity and clarity - make the James stuff a little bit less Jamesy (sorry, James) - Simplify special "No dep" column implementation. Trying to pretend it was a real package made things more complicated than necessary - Properly encode package names in fetch requests to handle scoped packages - Make the typahead Easter egg more difficult to trigger by accident
There was a problem hiding this comment.
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/pages/compare.vue (1)
2-58:⚠️ Potential issue | 🟠 MajorFix index misalignment when NO_DEPENDENCY_ID is not last.
gridColumnsfilterspackagesthen indexespackagesDataby the filtered index, so a URL likereact,no-dep,vuewill mis-map data and headers. Use the original index before filtering.Suggested fix
-const gridColumns = computed(() => - packages.value - .filter(pkg => pkg !== NO_DEPENDENCY_ID) - .map((pkg, i) => { - const data = packagesData.value?.[i] +const gridColumns = computed(() => + packages.value + .map((pkg, index) => ({ pkg, index })) + .filter(({ pkg }) => pkg !== NO_DEPENDENCY_ID) + .map(({ pkg, index }) => { + const data = packagesData.value?.[index] const header = data ? data.package.version ? `${data.package.name}@${data.package.version}` : data.package.name : pkg return { header, - replacement: replacements.value.get(pkg) ?? null, + replacement: replacements.value.get(pkg) ?? null, } }), )
🧹 Nitpick comments (3)
app/components/Compare/PackageSelector.vue (1)
108-112: Consider using i18n for the fallback label in aria-label.The fallback string
'No dependency'is hardcoded in English while the rest of the component uses i18n. For consistency and accessibility across locales, consider using the i18n key.♻️ Suggested fix
:aria-label=" $t('compare.selector.remove_package', { - package: pkg === NO_DEPENDENCY_ID ? 'No dependency' : pkg, + package: pkg === NO_DEPENDENCY_ID ? $t('compare.no_dependency.label') : pkg, }) "app/components/Package/Replacement.vue (1)
8-10: Thecommunityfield in the computed return value appears unused.The template at lines 74 renders the community text directly via
$t('package.replacement.community')rather than usingmessage[1].community. Thecommunityfield added to the computed return object is therefore redundant.Either remove the unused field from the computed, or update the template to use
message[1].communityfor consistency:Option 1: Remove unused field
case 'simple': return [ 'package.replacement.simple', { replacement: props.replacement.replacement, - community: $t('package.replacement.community'), }, ] case 'documented': - return [ - 'package.replacement.documented', - { - community: $t('package.replacement.community'), - }, - ] + return ['package.replacement.documented', {}]Option 2: Use the field in template
<template `#community`> <a href="https://e18e.dev/docs/replacements/" target="_blank" rel="noopener noreferrer" class="inline-flex items-center gap-1 ms-1 underline underline-offset-4 decoration-amber-600/60 dark:decoration-amber-400/50 hover:decoration-fg transition-colors" > - {{ $t('package.replacement.community') }} + {{ message[1].community }} <span class="i-carbon-launch w-3 h-3" aria-hidden="true" /> </a> </template>Also applies to: 25-26, 29-34
app/components/Tooltip/App.vue (1)
13-31: Consider clearing timeout on component unmount.If the component unmounts whilst
hideTimeoutis pending, the callback will attempt to update a reactive ref on a disposed component. This could cause a Vue warning in development.♻️ Suggested fix using onScopeDispose
const hideTimeout = shallowRef<ReturnType<typeof setTimeout> | null>(null) + +onScopeDispose(() => { + if (hideTimeout.value) { + clearTimeout(hideTimeout.value) + } +}) function show() {
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/composables/usePackageComparison.ts (1)
356-362:⚠️ Potential issue | 🟡 MinorEnsure
packageSizefacet handles no-dependency column consistently with other facets.The
packageSizecheckif (!data.packageSize)returnsnullwhenpackageSizeis0, causing the no-dependency column to display "–" rather than "0 B". This is inconsistent with the codebase pattern—facets likedownloads,moduleFormat, andtypesall explicitly checkisNoDependencyto provide appropriate display values. SincecreateNoDependencyData()setspackageSize: 0, add explicit handling:♻️ Suggested fix for packageSize no-dependency display
case 'packageSize': { - if (!data.packageSize) return null + if (data.packageSize === undefined || data.packageSize === null) return null + if (data.packageSize === 0 && isNoDependency) { + return { raw: 0, display: '0 B', status: 'neutral' } + } + if (data.packageSize === 0) return null return { raw: data.packageSize, display: formatBytes(data.packageSize), status: data.packageSize > 5 * 1024 * 1024 ? 'warning' : 'neutral', } }
🧹 Nitpick comments (1)
app/components/Tooltip/App.vue (1)
13-32: Consider clearing timeout on unmount to prevent potential memory leak.If the component unmounts whilst
hideTimeoutis pending, the timeout callback may execute on a stale component. Whilst Vue 3's reactivity system generally handles this gracefully, explicitly clearing the timeout inonBeforeUnmountwould be more defensive.♻️ Optional: Clear timeout on unmount
const hideTimeout = shallowRef<ReturnType<typeof setTimeout> | null>(null) + +onBeforeUnmount(() => { + if (hideTimeout.value) { + clearTimeout(hideTimeout.value) + } +}) function show() {
user needs to be able to reference the full details
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/compare.vue (1)
121-186:⚠️ Potential issue | 🟡 MinorRemove per‑button focus-visible utilities and rely on global focus styles.
These button classes override the global focus-visible rule. Please remove the inline
focus-visible:*utilities.Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components (e.g., AccessControls.vue). Rely on the global rule for consistency and maintainability; only use inline focus-visible utilities when styling non-button/select elements or in exceptional cases outside the global scope.Proposed fix
- <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" + <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 shrink-0" @@ - <button - type="button" - class="text-[10px] transition-colors focus-visible:outline-none focus-visible:underline focus-visible:underline-accent" + <button + type="button" + class="text-[10px] transition-colors" @@ - <button - type="button" - class="text-[10px] transition-colors focus-visible:outline-none focus-visible:underline focus-visible:underline-accent" + <button + type="button" + class="text-[10px] transition-colors"
4 participants
(note: this recording is a bit outdated. Play around with the deploy preview to see the latest.)
npmx.compare.with.module.replacements.v3.mp4
When selecting a package on the
/comparepage that has an entry in the e18e module-replacements database, show new CTAs:nativeandsimpletypes, show user the replacement info and include a CTA to add a "no dependency" baseline column for comparisondocumentedtype, it would be cool to suggest adding the actual package recommendation(s), but the current version ofmodule-replacementsdoes not have this as structured data. For now, just give general info in an alert and show "Learn more" linking to the specific replacement doc page.This also includes a bit of an Easter egg: