feat: extract button and link component, unify and improve design #1071
Conversation
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/nuxt/a11y.spec.ts (1)
358-388:⚠️ Potential issue | 🟡 Minor
disabled: truein variant-specificLinkBasetests means only the disabled<span>path is tested, not the interactive<NuxtLink>rendering.The "as secondary button", "as primary button", and "as small button" tests all pass
disabled: true(lines 360, 369, 380). When disabled,LinkBaserenders a plain<span>instead of<NuxtLink>, so the button-variant styling on the<NuxtLink>branch is never exercised by these tests. This is the same copy-paste issue that was previously flagged (and fixed) for theButtonBasevariant tests.Remove
disabled: trueto test the enabled/interactive state of each variant:Proposed fix
it('should have no accessibility violations as secondary button', async () => { const component = await mountSuspended(LinkBase, { - props: { to: 'http://example.com', disabled: true, variant: 'button-secondary' }, + props: { to: 'http://example.com', variant: 'button-secondary' }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations as primary button', async () => { const component = await mountSuspended(LinkBase, { - props: { to: 'http://example.com', disabled: true, variant: 'button-primary' }, + props: { to: 'http://example.com', variant: 'button-primary' }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations as small button', async () => { const component = await mountSuspended(LinkBase, { props: { to: 'http://example.com', - disabled: true, variant: 'button-secondary', size: 'small', }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) })
🧹 Nitpick comments (3)
app/components/Link/Base.vue (1)
62-73: Disabled button-variant span duplicates styling classes from the active NuxtLink block.The class strings for size/variant on the disabled
<span>(lines 65–71) are manually duplicated from the NuxtLink block (lines 81–88). If the button styling evolves, these two places can easily drift apart. Consider extracting shared class objects into computed properties to keep the disabled and active states in sync.app/pages/package/[[org]]/[name].vue (2)
19-19:ButtonBaseandButtonGroupare used in the template but not explicitly imported.
LinkBaseis explicitly imported from#components, butButtonBase(lines 609, 632) andButtonGroup(lines 648, 678) rely on Nuxt auto-import. For consistency, either import all three explicitly or remove the explicitLinkBaseimport and rely on auto-import for all.🔧 Suggested fix (if preferring explicit imports)
-import { LinkBase } from '#components' +import { LinkBase, ButtonBase, ButtonGroup } from '#components'
841-863: Duplicatev-ifconditions could be hoisted to the parent<span>.Both
LinkBaseelements (lines 845 and 856) share the samev-if="getDependencyCount(displayVersion) > 0"guard. Moving this condition to the wrapper<span>on line 841 would reduce duplication.♻️ Suggested refactor
- <span class="flex items-center gap-1"> + <span v-if="getDependencyCount(displayVersion) > 0" class="flex items-center gap-1"> <LinkBase variant="button-secondary" size="small" - v-if="getDependencyCount(displayVersion) > 0" :to="`https://npmgraph.js.org/?q=${pkg.name}`" :title="$t('package.stats.view_dependency_graph')" classicon="i-carbon:network-3" > <span class="sr-only">{{ $t('package.stats.view_dependency_graph') }}</span> </LinkBase> <LinkBase variant="button-secondary" size="small" - v-if="getDependencyCount(displayVersion) > 0" :to="`https://node-modules.dev/grid/depth#install=${pkg.name}${resolvedVersion ? `@${resolvedVersion}` : ''}`" :title="$t('package.stats.inspect_dependency_tree')" classicon="i-carbon:tree-view" > <span class="sr-only">{{ $t('package.stats.inspect_dependency_tree') }}</span> </LinkBase> </span>
|
@alexdln I think I addresse all your comments, except for the two I left unresovled. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
unifying button + link components is good, but I'm not a fan of the intensity of the gradient design on all buttons. is it possible to explore a 'flatter' feel that's more consistent with what we have now? though it might just be the white -> black gradient here that I mind - the dark themed buttons seem much nicer: 
note this, if no theme is selected: another point: I think we need consistent borders in the 'button bars' Screen.Recording.2026-02-06.at.12.58.06.mov |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Link/Base.vue (1)
90-91: Passing both:toand:hrefto NuxtLink is redundant.In Nuxt 3,
hrefis merely an alias forto, and when both are set,totakes precedence. Passingtoalone is sufficient. This is particularly inconsistent here since the component's own props explicitly declarehref?: never(line 38), with the comment "always usetoinstead ofhref", yet the code passeshrefdirectly to NuxtLink.Proposed simplification
:to="to" - :href="to" :external="isLinkExternal"
no worries, since this pr was intended to be a base component for link and button keep the old styles as it is. we can discuss & design them later to keep things managable |
|
@danielroe forgot to mention the border bug in the PR message. Just wanted to make sure this is the direction we want to go to, before fixing it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Link/Base.vue (2)
87-88: Hardcodedtext-blackon primary button may not contrast with all accent colours.If the accent colour is ever changed to a darker shade, black text will become unreadable. Consider using a contrast-aware token (e.g.
text-on-accentor a CSS custom property) instead of the literaltext-black.
90-91: Remove redundant:href="to"prop from NuxtLink.NuxtLink automatically derives the rendered
hrefattribute from thetoprop, regardless of whether:external="true". Since you already have:external="isLinkExternal"configured to handle external link rendering, the:hrefbinding is unnecessary and creates duplicate logic.Suggested fix
:to="to" - :href="to" :external="isLinkExternal"
|
@danielroe should I remove the gradient for now, so it doesn't block this PR? Then I can do another one where we discuss "more clickable looking buttons". Just wanted to showcase the general direction here, but that's not bound to the other changes. I can also take out the button group for now and redo that one. |
|
@essenmitsosse |
|
sure, let's take out the gradient for now. happy if you want to iterate on the button group - I think it's a nice UI touch. |
|
Removed the gradients and also the accent colors (for now). So it's closer to where we were before. Will move further redesign work to a later PR. This also solves the issue with the button group borders. 
|
|
Hmm, I am not sure about the header menu... Let's revert it for now too? And how a "like" looks like (same for deps). And the fact that npm/github/etc will open in a new tab is pretty obvious... P.S. Will it be possible to separate decomposition and design changes or it is critical for this PR already? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Button/Base.vue (1)
38-41: Per-elementfocus-visibleutility on a<button>— verify intent alongside global rule.The project applies
button:focus-visiblestyling globally inmain.css(outline + offset). This component additionally appliesfocus-visible:enabled:(bg-fg/30)as a background tint. If the intent is to layer both effects (outline from global + background from component), this is fine — but worth confirming it doesn't produce an unexpected double-treatment or clash. If the background tint is meant to replace the outline, consider adjusting the global rule instead.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."
| /** | ||
| * Don't use this directly, use `keyshortcut` instead. Correctly HTML will be automatically generated and the shortcut will automatically be displayed in the UI. | ||
| */ |
There was a problem hiding this comment.
JSDoc grammar still reads awkwardly after the typo fix.
The previous review corrected "Correcty" → "Correctly", but the word needed here is the adjective "Correct", not the adverb "Correctly". The sentence "Correctly HTML will be automatically generated" doesn't parse; it should be "Correct HTML will be automatically generated" (or "The correct HTML…").
Proposed fix
/**
- * Don't use this directly, use `keyshortcut` instead. Correctly HTML will be automatically generated and the shortcut will automatically be displayed in the UI.
+ * Don't use this directly, use `keyshortcut` instead. Correct HTML will be automatically generated and the shortcut will automatically be displayed in the UI.
*/| 'text-black bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50)': | ||
| variant === 'primary', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.ts" -o -name "*.js" -o -name "*.config.*" \) | grep -E "(uno|theme)" | head -20Repository: npmx-dev/npmx.dev
Length of output: 131
🏁 Script executed:
cat app/components/Button/Base.vueRepository: npmx-dev/npmx.dev
Length of output: 2101
🏁 Script executed:
find app -name "*.css" -o -name "*.vue" | xargs grep -l "data-theme\|--fg\|--bg" 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 262
🏁 Script executed:
cat app/assets/main.css | head -150Repository: npmx-dev/npmx.dev
Length of output: 4521
🏁 Script executed:
cat uno.config.tsRepository: npmx-dev/npmx.dev
Length of output: 7755
🏁 Script executed:
cat app/assets/main.css | grep -A5 "data-theme='light'" | head -40Repository: npmx-dev/npmx.dev
Length of output: 1071
🏁 Script executed:
python3 << 'EOF'
# Calculate WCAG contrast ratios for the identified scenarios
# Using OKLch lightness values which roughly correspond to relative luminance
# Light mode scenario
light_mode_bg_fg = 0.145 # oklch(0.145 0 0) = dark grey
light_mode_text_black = 0.0 # pure black
# Dark mode scenario
dark_mode_bg_fg = 0.985 # oklch(0.985 0 0) = very light
dark_mode_text_black = 0.0 # pure black
# Light mode with high contrast
light_mode_high_contrast_fg = 0.0 # oklch(0 0 0) = pure black
light_mode_high_contrast_text_black = 0.0 # pure black
# Light mode bg token (for text-bg alternative)
light_mode_bg_token = 1.0 # oklch(1 0 0) = white
# Dark mode bg token (for text-bg alternative)
dark_mode_bg_token = 0.145 # oklch(0.145 0 0) = dark
def estimate_contrast(lightness1, lightness2):
"""Rough WCAG contrast estimate using lightness values"""
# Simplified: contrast = |L1 - L2| / max(L1, L2) if they're similar
# For black vs similar dark, it's very poor
if lightness1 == lightness2 or (lightness1 < 0.2 and lightness2 < 0.2):
return 1.0 # Near 1:1 contrast (minimum)
return abs(lightness1 - lightness2)
print("=== CURRENT IMPLEMENTATION (text-black) ===")
print(f"Light mode: text-black on bg-fg (0.0 on 0.145) → Estimated contrast ratio: ~1.0 [FAIL]")
print(f"Dark mode: text-black on bg-fg (0.0 on 0.985) → Estimated contrast ratio: ~10 [PASS]")
print(f"Light mode (high contrast): text-black on fg (0.0 on 0.0) → Estimated contrast ratio: 1.0 [FAIL]")
print("\n=== PROPOSED FIX (text-bg) ===")
print(f"Light mode: text-bg on bg-fg (1.0 on 0.145) → Estimated contrast ratio: ~7 [PASS]")
print(f"Dark mode: text-bg on bg-fg (0.145 on 0.985) → Estimated contrast ratio: ~6.7 [PASS]")
print("\n=== OBSERVATIONS ===")
print("✓ text-bg exists in theme (maps to bg token)")
print("✓ text-bg is already used in codebase (aria-pressed state: 'bg-fg text-bg')")
print("✗ Current text-black creates poor contrast in light mode")
print("✓ Proposed text-bg fixes the issue while maintaining good contrast in both modes")
EOFRepository: npmx-dev/npmx.dev
Length of output: 853
🏁 Script executed:
# Verify that text-bg is being used elsewhere in the codebase
rg "text-bg" app/components/ --type=vueRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Search for text-bg usage in the entire app directory
rg "text-bg" app/Repository: npmx-dev/npmx.dev
Length of output: 3735
text-black on the primary variant causes accessibility failure in light mode.
In light mode, the --fg token is oklch(0.145 0 0) (dark grey), so text-black on bg-fg produces virtually no contrast—both are near-black. Use text-bg instead, which maps to the background token and automatically inverts: white text in light mode (excellent contrast on dark bg-fg) and dark text in dark mode (good contrast on light bg-fg). This pattern is already established throughout the codebase.
Suggested fix
- 'text-black bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50)':
+ 'text-bg bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50)':
variant === 'primary',📝 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.
| 'text-black bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50)': | |
| variant === 'primary', | |
| 'text-bg bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50)': | |
| variant === 'primary', |
|
there seems to be duplicate focus states here (double ring) Screen.Recording.2026-02-06.at.14.09.41.mov |
|
@danielroe what Browser is this? Is that on the preview or locally? |
|
this is firefox (well, Zen) and it's the preview |
|
@danielroe should be solved now |
There was a problem hiding this comment.
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 (2)
app/components/Package/Dependencies.vue (1)
225-236:⚠️ Potential issue | 🟡 MinorOptional deps "show all" button lost its styling classes.
The other two "show all" buttons (lines 133–144 and 181–192) have
font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded, but this button only hasmt-2 truncate. This looks like an accidental regression that will make this button visually inconsistent.Proposed fix
- class="mt-2 truncate" + class="mt-2 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded"test/nuxt/a11y.spec.ts (1)
358-388:⚠️ Potential issue | 🟡 Minor
disabled: truein variant-specific LinkBase tests likely unintentional.The "as secondary button" (line 360), "as primary button" (line 369), and "as small button" (line 380) tests all pass
disabled: true. This means you're testing the disabled rendering of these variants rather than their enabled state. Given the test descriptions don't mention "disabled", this appears to be a copy-paste artefact from the disabled state test above — the same pattern that was previously flagged and fixed for theButtonBasetests.Proposed fix
it('should have no accessibility violations as secondary button', async () => { const component = await mountSuspended(LinkBase, { - props: { to: 'http://example.com', disabled: true, variant: 'button-secondary' }, + props: { to: 'http://example.com', variant: 'button-secondary' }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations as primary button', async () => { const component = await mountSuspended(LinkBase, { - props: { to: 'http://example.com', disabled: true, variant: 'button-primary' }, + props: { to: 'http://example.com', variant: 'button-primary' }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations as small button', async () => { const component = await mountSuspended(LinkBase, { props: { to: 'http://example.com', - disabled: true, variant: 'button-secondary', size: 'small', }, slots: { default: 'Button link content' }, }) const results = await runAxe(component) expect(results.violations).toEqual([]) })
🧹 Nitpick comments (7)
app/components/Header/ConnectorModal.vue (1)
71-77: Disconnect button not migrated toButtonBase.The connect button (line 223) was migrated to
ButtonBase, but the disconnect button here remains a native<button>with extensive inline styling. Given the PR's goal of unifying interactive elements under the new component primitives, this appears to be an oversight.Additionally, the inline
focus-visible:outline-accent/70class on this button conflicts with the project convention of applying focus-visible styles globally viamain.css. Migrating toButtonBasewould likely resolve both issues.Suggested migration
- <button - type="button" - class="w-full px-4 py-2 font-mono text-sm text-fg-muted bg-bg-subtle border border-border rounded-md transition-colors duration-200 hover:text-fg hover:border-border-hover focus-visible:outline-accent/70" - `@click`="handleDisconnect" - > + <ButtonBase + type="button" + variant="secondary" + class="w-full" + `@click`="handleDisconnect" + > {{ $t('connector.modal.disconnect') }} - </button> + </ButtonBase>Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css… Individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
,
app/components/Package/Versions.vue (2)
561-563: Inlinefocus-visibleutility on button may conflict with the new global*:focus-visiblerule.Line 563 applies
focus-visible:outline-accent/70directly on this<button>. Since this PR changes the global focus styling to*:focus-visible, this inline utility could produce a duplicate or conflicting focus ring — consistent with the Firefox double-focus-ring issue reported by reviewers.This button should rely on the global rule for consistency. Consider removing the inline utility:
Proposed fix
<button type="button" - class="w-4 h-4 flex items-center justify-center text-fg-subtle hover:text-fg transition-colors shrink-0 focus-visible:outline-accent/70 rounded-sm" + class="w-4 h-4 flex items-center justify-center text-fg-subtle hover:text-fg transition-colors shrink-0 rounded-sm"Based on learnings: "In the npmx.dev project, 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 like
focus-visible:outline-accent/70."
415-429: Consider extracting aVersionLinksub-component to reduce template repetition.The same LinkBase pattern — with conditional deprecated styling (
text-red-400),classicon(i-carbon-warning-hex), deprecation-aware:title, anddir="ltr"version text — is repeated six times across this template. A smallVersionLink(or scoped slot/render helper) accepting aVersionDisplayprop would consolidate the duplicated bindings and make future styling changes a single-point edit.Not blocking for this PR, but worth considering in a follow-up.
Also applies to: 512-530, 704-718
app/components/Package/Dependencies.vue (1)
133-144: Remove inlinefocus-visible:outline-accent/70from button elements.Both "show all" buttons include
focus-visible:outline-accent/70in their class lists, but in this project focus-visible styling forbuttonelements is applied globally viamain.css. Per-element inline utilities are unnecessary and can cause inconsistencies.Proposed fix (dependencies button)
- class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70" + class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded"Proposed fix (peer dependencies button)
- class="mt-2 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70" + class="mt-2 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded"Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons and selects don't need inline focus-visible utility classes."
Also applies to: 181-192
app/pages/package/[[org]]/[name].vue (2)
19-19: Inconsistent explicit import:LinkBaseimported butButtonBase/ButtonGroupare auto-imported.
LinkBaseis explicitly imported from#components, yetButtonBaseandButtonGroup(used extensively in the template) rely on Nuxt auto-imports. Consider either importing all three explicitly for consistency, or removing this import and relying on auto-imports for all of them.
812-865: Deps stats section: verify visual consistency of inline icon-only buttons.The two
LinkBaseelements (dependency graph and dependency tree inspector) usevariant="button-secondary"withsize="small"and onlysr-onlytext content. Ensure these render as recognisable icon-only buttons with sufficient click targets. Also, thev-ifon line 847/858 is placed aftervariant/size— while Vue supports this, placingv-iffirst is the conventional ordering for readability.Proposed attribute reorder (optional)
<LinkBase - variant="button-secondary" - size="small" v-if="getDependencyCount(displayVersion) > 0" + variant="button-secondary" + size="small" :to="`https://npmgraph.js.org/?q=${pkg.name}`"app/pages/package-code/[...path].vue (1)
459-467: Unmigrated raw<a>tag for the "Raw" file view link.This native anchor with explicit
target="_blank"andrel="noopener noreferrer"was not migrated toLinkBase, unlike the similar CDN links at lines 494–500 and 546–552. Consider migrating for consistency —LinkBasewould handle the external-link behaviour automatically.Proposed fix
- <a - :href="`https://cdn.jsdelivr.net/npm/${packageName}@${version}/${filePath}`" - target="_blank" - rel="noopener noreferrer" - class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors inline-flex items-center gap-1" - > + <LinkBase + :to="`https://cdn.jsdelivr.net/npm/${packageName}@${version}/${filePath}`" + class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors inline-flex items-center gap-1" + > {{ $t('code.raw') }} - <span class="i-carbon:launch w-3 h-3" /> - </a> + </LinkBase>
5 participants
This one is quite aggressive. It does some additional work on the button and link components and replaces all buttons and links on the frontpage and package-page.
The basic idea behind this is to have a fixed set of component that can be used and will automatically have the desired designs. This way, if we want to redesign buttons/links later, we don't have to hunt them down in the whole code base. This should improve DX and also ensure Consistent design and a11y across the site.
This currently supports the following:
Additionally this does the following design changes:
The reason behind this is, that before a lot of clickable things where really hard to spot. This does add some visual noise, so I think it might be controversial. My line of thinking here is: Make the UI easy to understand and improve visual noise as a second step, instead of the other way around.
Additionally this aligns the static tag design with the button/link tag design, but ensures it is clear what is clickable and what is not.
Open Todos after this is merged:
I know this is tricky to review since it does changes on multiple levels (code structure, a11y, design etc.), so let me know if it would make sense to split this.