Skip to content

refactor: remove unnecessary prop from links #1247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alexdln merged 11 commits into from
Feb 9, 2026
Merged

refactor: remove unnecessary prop from links #1247

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f305156
fix: make sure button/link icons always have correct size
essenmitsosse Feb 8, 2026
37412df
fix: icons with to much spacing
essenmitsosse Feb 8, 2026
51b4fd5
feat: allow to make links block elements and make buttons blocks by d…
essenmitsosse Feb 8, 2026
a544c36
refactor: remove unnecessary element
essenmitsosse Feb 8, 2026
45a1a2f
fix: stop unnecessary tag truncation
essenmitsosse Feb 9, 2026
57f81b0
fix: broken logic
essenmitsosse Feb 9, 2026
322495f
fix: duplicate class
essenmitsosse Feb 9, 2026
86242b9
refactor: add explicit block prop to link and button comps
essenmitsosse Feb 9, 2026
1d1e74f
Merge remote-tracking branch 'upstream/main' into refactor-remove-unn…
essenmitsosse Feb 9, 2026
65bc1c3
Merge remote-tracking branch 'upstream/main' into refactor-remove-unn…
essenmitsosse Feb 9, 2026
af56682
refactor: use new prop for existing implementations
essenmitsosse Feb 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/AppHeader.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ onKeyStroke(
<!-- Mobile: Menu button (always visible, click to open menu) -->
<ButtonBase
type="button"
class="sm:hidden flex"
class="sm:hidden"
:aria-label="$t('nav.open_menu')"
:aria-expanded="showMobileMenu"
@click="showMobileMenu = !showMobileMenu"
Expand Down
8 changes: 2 additions & 6 deletions app/components/Button/Base.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ defineExpose({
<template>
<button
ref="el"
class="group cursor-pointer inline-flex gap-x-1 items-center justify-center font-mono border border-border rounded-md transition-all duration-200 disabled:(opacity-40 cursor-not-allowed border-transparent)"
class="group cursor-pointer flex gap-x-1 items-center justify-center font-mono border border-border rounded-md transition-all duration-200 disabled:(opacity-40 cursor-not-allowed border-transparent)"
:class="{
'text-sm px-4 py-2': size === 'medium',
Comment thread
alexdln marked this conversation as resolved.
'text-xs px-2 py-0.5': size === 'small',
Expand All @@ -53,11 +53,7 @@ defineExpose({
"
:aria-keyshortcuts="keyshortcut"
>
<span
v-if="classicon"
:class="[size === 'small' ? 'size-3' : 'size-4', classicon]"
aria-hidden="true"
/>
<span v-if="classicon" class="size-[1em]" :class="classicon" aria-hidden="true" />
Comment thread
alexdln marked this conversation as resolved.
<slot />
<kbd
v-if="keyshortcut"
Expand Down
43 changes: 19 additions & 24 deletions app/components/Link/Base.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ const props = withDefaults(
* `type` should never be used, because this will always be a link.
* */
'type'?: never
'variant'?: 'button-primary' | 'button-secondary' | 'link'
'variant'?: 'button-primary' | 'button-secondary' | 'link' | 'link-block'
Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026

Choose a reason for hiding this comment

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

Don't mix responsibilities. The prop should be responsible for a specific area

Copy link
Copy Markdown
Contributor Author

@essenmitsosse essenmitsosse Feb 9, 2026

Choose a reason for hiding this comment

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

I know what you mean. This goes back to my previous comment: Adding another prop, that is only relevant for a certain variant is also not the best design. Again: probably best to split the component. Would you be fine with this as an intermediate solution?

Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Let's call this inline already and use it for both buttons and links

Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026

Choose a reason for hiding this comment

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

Sorry, missed comment

Copy link
Copy Markdown
Contributor Author

@essenmitsosse essenmitsosse Feb 9, 2026

Choose a reason for hiding this comment

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

Yeah I see. Just one thing: Would it default to true for links but false for buttons?

Copy link
Copy Markdown
Contributor Author

@essenmitsosse essenmitsosse Feb 9, 2026

Choose a reason for hiding this comment

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

Ah, just noticed buttons are currently inline as well. Ok I'll have a go on it.

Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026

Choose a reason for hiding this comment

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

The point isn't that there are so many options, but that they're for completely different purposes. In your set of conditions below, you can see what you're using for what. Essentially, you're generating several other props from this one

And this list of conditions below is actually more logical for DX, and that's what you came up with. We're more comfortable writing and working with a component when we clearly understand what the result will be

false by default please. It's easier and more common to write just inline, not inline="false"

Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026

Choose a reason for hiding this comment

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

This is also pretty standard practice when it's responsible for a narrow section: disabled, autocorrect, checked, hidden, open. It's probably even more standard in Vue, since it's positioned as closer to the HTML

Copy link
Copy Markdown
Member

@alexdln alexdln Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

block is better, yes (but now we've run into a terminology conflict. Would be better to work on that in the next iteration)

Copy link
Copy Markdown
Contributor Author

@essenmitsosse essenmitsosse Feb 9, 2026

Choose a reason for hiding this comment

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

Yeah, never would have liked to set true as the default for the reasons you stated. What terminology conflict do you mean?

'size'?: 'small' | 'medium'
'iconSize'?: 'sm' | 'md' | 'lg'

'keyshortcut'?: string

Expand Down Expand Up @@ -52,28 +51,21 @@ const isLinkAnchor = computed(
() => !!props.to && typeof props.to === 'string' && props.to.startsWith('#'),
)

const ICON_SIZE_MAP = {
sm: 'size-3 min-w-3',
md: 'size-4 min-w-4',
lg: 'size-5 min-w-5',
}

/** size is only applicable for button like links */
const isLink = computed(() => props.variant === 'link')
const isButton = computed(() => props.variant !== 'link')
const isButtonSmall = computed(() => props.size === 'small' && props.variant !== 'link')
const isButtonMedium = computed(() => props.size === 'medium' && props.variant !== 'link')

const iconSizeClass = computed(
() => ICON_SIZE_MAP[props.iconSize || (isButtonSmall.value && 'sm') || 'md'],
)
const isLink = computed(() => props.variant === 'link' || props.variant === 'link-block')
const isButton = computed(() => !isLink.value)
const isButtonSmall = computed(() => props.size === 'small' && !isLink.value)
const isButtonMedium = computed(() => props.size === 'medium' && !isLink.value)
const isBlock = computed(() => isButton.value || props.variant === 'link-block')
Comment thread
alexdln marked this conversation as resolved.
Outdated
</script>

<template>
<span
v-if="disabled"
:class="{
'opacity-50 inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md':
'flex': isBlock,
'inline-flex': !isBlock,
'opacity-50 gap-x-1 items-center justify-center font-mono border border-transparent rounded-md':
isButton,
'text-sm px-4 py-2': isButtonMedium,
'text-xs px-2 py-0.5': isButtonSmall,
Expand All @@ -84,12 +76,15 @@ const iconSizeClass = computed(
/></span>
<NuxtLink
v-else
class="group/link inline-flex gap-x-1 items-center justify-center"
class="group/link gap-x-1 items-center"
:class="{
'flex': isBlock,
'inline-flex': !isBlock,
Comment thread
essenmitsosse marked this conversation as resolved.
Outdated
'underline-offset-[0.2rem] underline decoration-1 decoration-fg/30': !isLinkAnchor && isLink,
'font-mono text-fg hover:(decoration-accent text-accent) focus-visible:(decoration-accent text-accent) transition-colors duration-200':
'justify-start font-mono text-fg hover:(decoration-accent text-accent) focus-visible:(decoration-accent text-accent) transition-colors duration-200':
isLink,
'font-mono border border-border rounded-md transition-all duration-200': isButton,
'justify-center font-mono border border-border rounded-md transition-all duration-200':
isButton,
'text-sm px-4 py-2': isButtonMedium,
'text-xs px-2 py-0.5': isButtonSmall,
'bg-transparent text-fg hover:(bg-fg/10) focus-visible:(bg-fg/10)':
Expand All @@ -100,22 +95,22 @@ const iconSizeClass = computed(
:aria-keyshortcuts="keyshortcut"
:target="isLinkExternal ? '_blank' : undefined"
>
<span v-if="classicon" class="me-1" :class="[iconSizeClass, classicon]" aria-hidden="true" />
<span v-if="classicon" class="size-[1em]" :class="classicon" aria-hidden="true" />
<slot />
<!-- automatically show icon indicating external link -->
<span
v-if="isLinkExternal && !classicon"
class="i-carbon:launch rtl-flip w-3 h-3 opacity-50"
class="i-carbon:launch rtl-flip size-[1em] opacity-50"
aria-hidden="true"
/>
<span
v-else-if="isLinkAnchor && isLink"
class="i-carbon:link w-3 h-3 opacity-0 group-hover/link:opacity-100 transition-opacity duration-200"
class="i-carbon:link size-[1em] opacity-0 group-hover/link:opacity-100 transition-opacity duration-200"
aria-hidden="true"
/>
<kbd
v-if="keyshortcut"
class="ms-2 inline-flex items-center justify-center w-4 h-4 text-xs text-fg bg-bg-muted border border-border rounded no-underline"
class="ms-2 inline-flex items-center justify-center size-4 text-xs text-fg bg-bg-muted border border-border rounded no-underline"
aria-hidden="true"
>
{{ keyshortcut }}
Expand Down
74 changes: 36 additions & 38 deletions app/components/Package/Versions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,33 +354,31 @@ function getTagVersions(tag: string): VersionDisplay[] {
<!-- Version info -->
<div class="flex-1 py-1.5 min-w-0 flex gap-2 justify-between items-center">
<div class="overflow-hidden">
<div>
<LinkBase
:to="versionRoute(row.primaryVersion.version)"
class="text-sm block truncate"
:class="
row.primaryVersion.deprecated ? 'text-red-400 hover:text-red-300' : undefined
"
:title="
row.primaryVersion.deprecated
? $t('package.versions.deprecated_title', {
version: row.primaryVersion.version,
})
: row.primaryVersion.version
"
:classicon="row.primaryVersion.deprecated ? 'i-carbon-warning-hex' : undefined"
icon-size="sm"
>
<span dir="ltr">
{{ row.primaryVersion.version }}
</span>
</LinkBase>
</div>
<LinkBase
:to="versionRoute(row.primaryVersion.version)"
variant="link-block"
class="text-sm"
:class="
row.primaryVersion.deprecated ? 'text-red-400 hover:text-red-300' : undefined
"
:title="
row.primaryVersion.deprecated
? $t('package.versions.deprecated_title', {
version: row.primaryVersion.version,
})
: row.primaryVersion.version
"
:classicon="row.primaryVersion.deprecated ? 'i-carbon-warning-hex' : undefined"
>
<span dir="ltr" class="block truncate">
{{ row.primaryVersion.version }}
</span>
</LinkBase>
<div v-if="row.tags.length" class="flex items-center gap-1 mt-0.5 flex-wrap">
<span
v-for="tag in row.tags"
:key="tag"
class="text-4xs font-semibold text-fg-subtle uppercase tracking-wide truncate max-w-[150px]"
class="text-4xs font-semibold text-fg-subtle uppercase tracking-wide truncate"
:title="tag"
>
{{ tag }}
Expand Down Expand Up @@ -415,17 +413,17 @@ function getTagVersions(tag: string): VersionDisplay[] {
<div class="flex items-center justify-between gap-2">
<LinkBase
:to="versionRoute(v.version)"
class="text-xs block truncate"
variant="link-block"
class="text-xs"
:class="v.deprecated ? 'text-red-400 hover:text-red-300' : undefined"
:title="
v.deprecated
? $t('package.versions.deprecated_title', { version: v.version })
: v.version
"
:classicon="v.deprecated ? 'i-carbon-warning-hex' : undefined"
icon-size="sm"
>
<span dir="ltr">
<span dir="ltr" class="block truncate">
{{ v.version }}
</span>
</LinkBase>
Expand Down Expand Up @@ -513,7 +511,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
<div class="flex items-center justify-between gap-2">
<LinkBase
:to="versionRoute(row.primaryVersion.version)"
class="text-xs block truncate"
variant="link-block"
class="text-xs"
:class="
row.primaryVersion.deprecated ? 'text-red-400 hover:text-red-300' : undefined
"
Expand All @@ -525,9 +524,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
: row.primaryVersion.version
"
:classicon="row.primaryVersion.deprecated ? 'i-carbon-warning-hex' : undefined"
icon-size="sm"
>
<span dir="ltr">
<span dir="ltr" class="block truncate">
{{ row.primaryVersion.version }}
</span>
</LinkBase>
Expand Down Expand Up @@ -586,7 +584,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
<LinkBase
v-if="group.versions[0]?.version"
:to="versionRoute(group.versions[0]?.version)"
class="text-xs block truncate"
variant="link-block"
class="text-xs"
:class="
group.versions[0]?.deprecated
? 'text-red-400 hover:text-red-300'
Expand All @@ -602,9 +601,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
:classicon="
group.versions[0]?.deprecated ? 'i-carbon-warning-hex' : undefined
"
icon-size="sm"
>
<span dir="ltr">
<span dir="ltr" class="block truncate">
{{ group.versions[0]?.version }}
</span>
</LinkBase>
Expand Down Expand Up @@ -644,11 +642,11 @@ function getTagVersions(tag: string): VersionDisplay[] {
<div v-else class="py-1">
<div class="flex items-center justify-between gap-2">
<div class="flex items-center gap-2 min-w-0">
<span class="w-4 shrink-0" />
<LinkBase
v-if="group.versions[0]?.version"
:to="versionRoute(group.versions[0]?.version)"
class="text-xs block truncate"
variant="link-block"
class="text-xs ms-6"
:class="
group.versions[0]?.deprecated
? 'text-red-400 hover:text-red-300'
Expand All @@ -664,9 +662,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
:classicon="
group.versions[0]?.deprecated ? 'i-carbon-warning-hex' : undefined
"
icon-size="sm"
>
<span dir="ltr">
<span dir="ltr" class="block truncate">
{{ group.versions[0]?.version }}
</span>
</LinkBase>
Expand Down Expand Up @@ -708,7 +705,8 @@ function getTagVersions(tag: string): VersionDisplay[] {
<div class="flex items-center justify-between gap-2">
<LinkBase
:to="versionRoute(v.version)"
class="text-xs block truncate"
variant="link-block"
class="text-xs"
:class="v.deprecated ? 'text-red-400 hover:text-red-300' : undefined"
:title="
v.deprecated
Expand All @@ -717,7 +715,7 @@ function getTagVersions(tag: string): VersionDisplay[] {
"
:classicon="v.deprecated ? 'i-carbon-warning-hex' : undefined"
>
<span dir="ltr">
<span dir="ltr" class="block truncate">
{{ v.version }}
</span>
</LinkBase>
Expand Down
2 changes: 1 addition & 1 deletion app/components/Tag/Static.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const props = withDefaults(
class="bg-bg-muted text-fg-muted inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md text-xs px-2 py-0.5"
:class="{ 'opacity-40': variant === 'ghost' }"
>
<span v-if="classicon" :class="['size-3', classicon]" aria-hidden="true" />
<span v-if="classicon" class="size-[1em]" :class="classicon" aria-hidden="true" />
<slot />
</component>
</template>
Loading