Skip to content

Comments

refactor: remove unnecessary prop from links#1247

Merged
alexdln merged 11 commits intonpmx-dev:mainfrom
essenmitsosse:refactor-remove-unnecessary-prop-from-links
Feb 9, 2026
Merged

refactor: remove unnecessary prop from links#1247
alexdln merged 11 commits intonpmx-dev:mainfrom
essenmitsosse:refactor-remove-unnecessary-prop-from-links

Conversation

@essenmitsosse
Copy link
Contributor

In #1240 @alexdln fixed two regressions I had introduce in #1071:

  1. a regression with the icon size in links (for some versions on the package page)
  2. a regression with the truncation of long version names

Unfortunatly that did two things:

  • by fixing 1.: introduce a new prop for the icon size, that would manually be set when changing the font size of a link (which would still make it easy to use the component and end up with a broken design)
  • by fixing 2.: added an explicit margin to all icons in links, which broke icon only links
    Screenshot 2026-02-09 at 00 53 42

This fixes this by:

  • making the icon size relative to the font size of a link/button, no matter how the font size was set. This makes the components more robust and easier to use while ensuring a consistent design. This might still be insufficient in some cases, but not in any we currently have. The result should be identical to the fix @alexdln introduced
  • refactor the links in /Package/Versions so that truncate works, but the links can keep their flex layout

To avoid flex! on links it additionally:

  • adds a new link-block variant, to properly let a link be a block element, as well as changing all buttons and links with button design to be blocks by default (couldn't find any case where this would be a problem)

It also fixes a very rare case were the tag name would be truncated without need.

@alexdln thanks for spotting the regression! Would you be so kind and double check, I didn't make things worse again and missed anything?

Preview:

Screenshot 2026-02-09 at 01 12 56

@vercel
Copy link

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

Request Review

@codecov
Copy link

codecov bot commented Feb 9, 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!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

Adds a block?: boolean prop to Button/Base.vue and Link/Base.vue, removes the iconSize prop and related ICON_SIZE_MAP logic from Link/Base.vue, and replaces size-dependent icon classes with a fixed size-[1em] class in Button, Link and Tag components. Button and Link root classes now switch between inline-flex and flex based on the new block prop; Link sizing logic was refactored to be block-aware (isButton, isButtonSmall/Medium). Multiple component usages were updated to pass block or remove inline-flex classes; no other public API changes beyond the added/removed props.

Possibly related PRs

Suggested labels

front

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly relates to the changeset, explaining the rationale for adding a block prop to buttons and links, fixing icon sizing issues, and addressing regressions from previous changes.

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

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
app/components/Link/Base.vue (1)

56-57: Consider using isButton.value for consistency.

isButtonSmall and isButtonMedium duplicate the !isLink.value check rather than reusing isButton.value. While functionally equivalent, using isButton.value would be more maintainable if the button detection logic ever changes.

♻️ Suggested refactor
-const isButtonSmall = computed(() => props.size === 'small' && !isLink.value)
-const isButtonMedium = computed(() => props.size === 'medium' && !isLink.value)
+const isButtonSmall = computed(() => props.size === 'small' && isButton.value)
+const isButtonMedium = computed(() => props.size === 'medium' && isButton.value)

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 (1)
app/components/Package/Versions.vue (1)

370-370: Consider using colon-syntax for the UnoCSS icon.

The icon class i-carbon-warning-hex should use the colon-separated form i-carbon:warning-hex for better UnoCSS resolution performance. This applies to all similar occurrences in this file (lines 423, 525, 601, 662, 715).

♻️ Suggested change
-:classicon="row.primaryVersion.deprecated ? 'i-carbon-warning-hex' : undefined"
+:classicon="row.primaryVersion.deprecated ? 'i-carbon:warning-hex' : undefined"

Based on learnings: "prefer colon-syntax for icons (e.g., i-carbon:checkmark) over the dash-separated form (i-carbon-checkmark). This aids UnoCSS in resolving the correct collection directly."

@danielroe danielroe requested a review from alexdln February 9, 2026 00:21
@essenmitsosse essenmitsosse marked this pull request as draft February 9, 2026 00:25
* */
'type'?: never
'variant'?: 'button-primary' | 'button-secondary' | 'link'
'variant'?: 'button-primary' | 'button-secondary' | 'link' | 'link-block'
Copy link
Member

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
Contributor Author

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

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

Copy link
Member

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
Contributor Author

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
Contributor Author

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
Member

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
Member

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

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
Contributor Author

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?

@essenmitsosse
Copy link
Contributor Author

@alexdln Thanks for the input. You were right about the prop being overloaded. I went with having the components be inline (inline-flex by default with the prop block changing them to be block elements (flex).

I would handle the inline-inline links separately and opened an issue for the them (#1252).

Are you fine with this?

@alexdln
Copy link
Member

alexdln commented Feb 9, 2026

How about putting the rest of the comments there too (or in another issue)?

@alexdln
Copy link
Member

alexdln commented Feb 9, 2026

Looks good to me, thanks for working on it and polishing it 🩵

@essenmitsosse
Copy link
Contributor Author

@alexdln I think I got all your comments that are issue worthy. Thanks for the input!

@alexdln
Copy link
Member

alexdln commented Feb 9, 2026

@essenmitsosse Please update issues descriptions a bit so that newcomers to the project can more easily understand what happened - in case someone will want to take it right away

Merged via the queue into npmx-dev:main with commit 2a474e9 Feb 9, 2026
16 checks passed
@alexdln
Copy link
Member

alexdln commented Feb 9, 2026

@essenmitsosse Also, to rework the props in general. Variant still has too much responsibility.

One for color, one for type (link / button)

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.

3 participants