refactor: remove unnecessary prop from links#1247
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a Possibly related PRs
Suggested labels
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
There was a problem hiding this comment.
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-hexshould use the colon-separated formi-carbon:warning-hexfor 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."
119c3a3 to
57f81b0
Compare
app/components/Link/Base.vue
Outdated
| * */ | ||
| 'type'?: never | ||
| 'variant'?: 'button-primary' | 'button-secondary' | 'link' | ||
| 'variant'?: 'button-primary' | 'button-secondary' | 'link' | 'link-block' |
There was a problem hiding this comment.
Don't mix responsibilities. The prop should be responsible for a specific area
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Let's call this inline already and use it for both buttons and links
There was a problem hiding this comment.
Yeah I see. Just one thing: Would it default to true for links but false for buttons?
There was a problem hiding this comment.
Ah, just noticed buttons are currently inline as well. Ok I'll have a go on it.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
block is better, yes (but now we've run into a terminology conflict. Would be better to work on that in the next iteration)
There was a problem hiding this comment.
Yeah, never would have liked to set true as the default for the reasons you stated. What terminology conflict do you mean?
|
@alexdln Thanks for the input. You were right about the prop being overloaded. I went with having the components be inline ( I would handle the inline-inline links separately and opened an issue for the them (#1252). Are you fine with this? |
|
How about putting the rest of the comments there too (or in another issue)? |
…ecessary-prop-from-links
|
Looks good to me, thanks for working on it and polishing it 🩵 |
|
@alexdln I think I got all your comments that are issue worthy. Thanks for the input! |
|
@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 |
|
@essenmitsosse Also, to rework the props in general. Variant still has too much responsibility. One for color, one for type (link / button) |
In #1240 @alexdln fixed two regressions I had introduce in #1071:
Unfortunatly that did two things:
This fixes this by:
/Package/Versionsso that truncate works, but the links can keep their flex layoutTo avoid
flex!on links it additionally:link-blockvariant, 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: