-
-
Notifications
You must be signed in to change notification settings - Fork 237
refactor(LinkBase/ButtonBase): add inline prop & rename size prop for buttons and links #1319
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request refactors the LinkBase component's public API by replacing the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this 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 (3)
app/components/Link/Base.vue (1)
54-78: Add exhaustive handling or a default case to the switch statements.The
sizeClasscomputed property has two switch statements that don't handle the exhaustive case. While TypeScript will catch invalid values at compile time, adding exhaustive handling improves runtime safety and makes the code more maintainable.♻️ Proposed fix to add exhaustive type checking
const sizeClass = computed(() => { if (isButton.value) { switch (props.size) { case 'xs': return 'text-xs px-2 py-0.5' case 'sm': return 'text-sm px-4 py-2' case 'md': return 'text-base px-5 py-2.5' case 'lg': return 'text-lg px-6 py-3' + default: { + const _exhaustiveCheck: never = props.size + return _exhaustiveCheck + } } } switch (props.size) { case 'xs': return 'text-xs' case 'sm': return 'text-sm' case 'md': return 'text-base' case 'lg': return 'text-lg' + default: { + const _exhaustiveCheck: never = props.size + return _exhaustiveCheck + } } })As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
app/components/Package/MetricsBadges.vue (1)
61-66: Switch UnoCSS icon class to colon syntax.
This keeps icon resolution consistent with the preset-icons convention and avoids the slower dash lookup.♻️ Suggested change
- classicon="i-carbon-checkmark" + classicon="i-carbon:checkmark"Based on learnings: In Vue components that use UnoCSS with the preset-icons collection, prefer colon-syntax for icons (e.g., i-carbon:checkmark) over the dash-separated form (i-carbon-checkmark).
app/components/Package/Versions.vue (1)
646-651: Consider using the new size prop for the single-version link.
This keeps sizing consistent with the rest of the file and avoids relying on manual text-size classes.♻️ Suggested change
- <LinkBase - v-if="group.versions[0]?.version" - :to="versionRoute(group.versions[0]?.version)" - :inline="false" - class="text-xs ms-6" + <LinkBase + v-if="group.versions[0]?.version" + :to="versionRoute(group.versions[0]?.version)" + :inline="false" + size="xs" + class="ms-6"
| <LinkBase :to="scriptParts[scriptName].link" text="sm"> | ||
| {{ scriptParts[scriptName].filePath }} | ||
| </LinkBase> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely typo: text="sm" should be size="sm".
Based on the PR objectives and all other LinkBase usages in this PR (which use size="xs" or size="sm"), this appears to be a typo. The text prop doesn't align with the documented LinkBase API changes.
🔧 Proposed fix
{{ scriptParts[scriptName].prefix }}
- <LinkBase :to="scriptParts[scriptName].link" text="sm">
+ <LinkBase :to="scriptParts[scriptName].link" size="sm">
{{ scriptParts[scriptName].filePath }}
</LinkBase>📝 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.
| <LinkBase :to="scriptParts[scriptName].link" text="sm"> | |
| {{ scriptParts[scriptName].filePath }} | |
| </LinkBase> | |
| <LinkBase :to="scriptParts[scriptName].link" size="sm"> | |
| {{ scriptParts[scriptName].filePath }} | |
| </LinkBase> |
|
Thanks for your work. Won't have time to fully review it until later. Just one thing: I think |
| const sizeClass = computed(() => { | ||
| if (isButton.value) { | ||
| switch (props.size) { | ||
| case 'xs': | ||
| return 'text-xs px-2 py-0.5' | ||
| case 'sm': | ||
| return 'text-sm px-4 py-2' | ||
| case 'md': | ||
| return 'text-base px-5 py-2.5' | ||
| case 'lg': | ||
| return 'text-lg px-6 py-3' | ||
| } | ||
| } | ||
|
|
||
| switch (props.size) { | ||
| case 'xs': | ||
| return 'text-xs' | ||
| case 'sm': | ||
| return 'text-sm' | ||
| case 'md': | ||
| return 'text-base' | ||
| case 'lg': | ||
| return 'text-lg' | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a class like this doesn't work, because Uno won't pick these up. It only works "by accident", because these classes are all used somewhere else. Try setting bg-indigo-300 here and it wouldn't be applied.
This needs to be inlined into the components class prop.
|
The |
resolves: #1254