feat: migrate to shadcn Button #179
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplaces ESLint auto-import config with Biome/biomelintrc entries, adds a new Button component, class utility and design system (Reka UI, Lucide icons, Tailwind theme), updates TypeScript paths and auto-import/type declarations, and refactors ~20+ frontend components to use the new Button and icon slots. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageComponent as Component
participant ButtonComponent as Button
participant Variants as buttonVariants
participant CN as cn (clsx+twMerge)
Note over ButtonComponent Variants CN #DDEBF7: New Button render & class composition flow
User->>PageComponent: clicks action (Add/Delete/Refresh)
PageComponent->>ButtonComponent: render with props (variant, size, class) + slot icon/text
ButtonComponent->>Variants: compute variant classes
Variants-->>ButtonComponent: variant class string
ButtonComponent->>CN: merge classes (base + variant + passed class)
CN-->>ButtonComponent: final classString
ButtonComponent->>User: emit click / perform action (UI updates)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue (1)
156-166: Remove unused PrimeVue helper functions.The helper functions
getSaveButtonIcon()andgetSaveButtonSeverity()return PrimeVue-specific values (PrimeReact icon class names and severity strings) that are no longer used by the new Button component. After migrating the Save button to use inline conditional icons (see previous comment), these functions should be removed.Apply this diff to remove the unused helpers:
-const getSaveButtonIcon = () => { - if (isSaving.value) return 'pi pi-spin pi-spinner' - if (saveStatus.value === 'success' && !schemaStore.isDirty) return 'pi pi-check' - if (saveStatus.value === 'error') return 'pi pi-times' - return 'pi pi-save' -} - -const getSaveButtonSeverity = () => { - if (saveStatus.value === 'success' && !schemaStore.isDirty) return 'success' - if (saveStatus.value === 'error') return 'danger' - return undefined -} -
🧹 Nitpick comments (5)
biome.json (1)
73-73: Verify that disablingnoUnusedImportsfor Vue files is intentional.Disabling
noUnusedImportsin addition tonoUnusedVariablesfor all Vue files will suppress warnings about unused imports, which could mask actual issues where developers manually add imports that are never used.If this is specifically to accommodate auto-imports (which generate imports automatically), consider using a more targeted approach or documenting this decision.
frontend/src/features/wikibase-schema/components/StatementManager.vue (1)
34-40: Specify explicit Button variant for consistency.This Button lacks a
variantprop, making it rely on the default variant. For consistency with other buttons in this PR (e.g., PropertyValueList.vue usesvariant="outline"for add actions), specify an explicit variant.Apply this diff:
<Button size="sm" + variant="outline" @click="handleAddStatement" > <LucidePlus /> Add Statement </Button>frontend/src/shared/components/Header.vue (1)
27-30: Specify explicit Button size for consistency.This Button lacks a
sizeprop. For consistency with other buttons throughout this PR (which predominantly usesize="sm"), specify an explicit size.Apply this diff:
- <Button variant="outline"> + <Button variant="outline" size="sm"> <LucideMoveLeft /> Back to Projects </Button>frontend/src/components/ui/button/index.ts (1)
6-34: LGTM! buttonVariants definition is comprehensive and well-designed.The
cvaconfiguration provides:
- Robust base styling with focus states, disabled states, and SVG size handling
- Six semantic variant options with appropriate color schemes
- Seven size options including dedicated icon button sizes
- Smart use of
has-[>svg]selector to adjust padding when icons are present- Dark mode support for relevant variants
- Sensible defaults (
variant: 'default',size: 'default')The base classes string on line 7 is quite long but follows the common pattern for shadcn-style button variants.
If you prefer improved readability, consider formatting the base classes string across multiple lines:
export const buttonVariants = cva( [ "inline-flex items-center justify-center gap-2 whitespace-nowrap rounded-md", "text-sm font-medium transition-all", "disabled:pointer-events-none disabled:opacity-50", "[&_svg]:pointer-events-none [&_svg:not([class*='size-'])]:size-4", "shrink-0 [&_svg]:shrink-0 outline-none", "focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]", "aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive" ].join(" "), { // ... rest of config } )frontend/src/components/ui/button/Button.vue (1)
23-23: Thehover:cursor-pointerclass is optional but not harmful.Based on verification,
buttonVariantsdoes not include cursor styling. For native<button>elements, the cursor pointer is implicit browser behavior, making this class technically redundant. However, since<Primitive>can render as different elements (controlled by theasprop), keeping the explicit class ensures consistent pointer styling across variants and maintains defensive styling. Removing it is safe for the default button behavior but optional—this is a style consistency preference rather than a necessity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.biomeignore(0 hunks)biome.json(1 hunks)frontend/.biomelintrc-auto-import.json(1 hunks)frontend/.eslintrc-auto-import.json(0 hunks)frontend/auto-imports.d.ts(12 hunks)frontend/components.d.ts(1 hunks)frontend/components.json(1 hunks)frontend/interface-extensions.d.ts(0 hunks)frontend/package.json(2 hunks)frontend/src/assets/base.css(1 hunks)frontend/src/components/ui/button/Button.vue(1 hunks)frontend/src/components/ui/button/index.ts(1 hunks)frontend/src/features/data-processing/components/ColumnHeaderMenu.vue(2 hunks)frontend/src/features/data-processing/components/ReplaceDialog.vue(2 hunks)frontend/src/features/project-management/pages/OpenProject.vue(4 hunks)frontend/src/features/wikibase-schema/components/ClaimEditor.vue(3 hunks)frontend/src/features/wikibase-schema/components/DropZone.vue(2 hunks)frontend/src/features/wikibase-schema/components/PropertyValueItem.vue(2 hunks)frontend/src/features/wikibase-schema/components/PropertyValueList.vue(2 hunks)frontend/src/features/wikibase-schema/components/QualifiersEditor.vue(3 hunks)frontend/src/features/wikibase-schema/components/ReferenceContainer.vue(2 hunks)frontend/src/features/wikibase-schema/components/ReferencesEditor.vue(5 hunks)frontend/src/features/wikibase-schema/components/SchemaSelector.vue(3 hunks)frontend/src/features/wikibase-schema/components/StatementEditor.vue(4 hunks)frontend/src/features/wikibase-schema/components/StatementManager.vue(2 hunks)frontend/src/features/wikibase-schema/components/TermSection.vue(2 hunks)frontend/src/features/wikibase-schema/components/ValidationDisplay.vue(5 hunks)frontend/src/features/wikibase-schema/components/ValueMappingEditor.vue(1 hunks)frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue(5 hunks)frontend/src/features/wikibase-schema/composables/useValueMapping.ts(2 hunks)frontend/src/lib/utils.ts(1 hunks)frontend/src/shared/components/Header.vue(2 hunks)frontend/tsconfig.json(1 hunks)frontend/vite.config.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/interface-extensions.d.ts
- .biomeignore
- frontend/.eslintrc-auto-import.json
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/ui/button/index.ts (1)
frontend/auto-imports.d.ts (1)
ButtonVariants(415-415)
frontend/auto-imports.d.ts (3)
frontend/src/shared/types/wikibase-schema.ts (1)
ValidationMessages(40-49)frontend/src/components/ui/button/index.ts (1)
buttonVariants(6-34)frontend/src/lib/utils.ts (1)
cn(5-7)
🪛 GitHub Actions: Typecheck code
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue
[error] 177-177: TypeScript error TS7006: Parameter 'event' implicitly has an 'any' type.
🔇 Additional comments (33)
frontend/tsconfig.json (1)
17-18: LGTM!The path mapping update correctly simplifies the
@frontend/*alias to be relative to the frontend directory, while maintaining the appropriate relative path for@backend/*.frontend/package.json (1)
9-9: LGTM!The new dependencies and
genscript appropriately support the shadcn-vue migration. The package versions align with current stable releases.Based on learnings: The versions match the documented stable releases for class-variance-authority (0.7.1), clsx (2.1.1), tailwind-merge (3.3.1), and lucide-vue-next (0.5xx series).
Also applies to: 18-19, 22-22, 26-27, 29-29
frontend/src/assets/base.css (1)
3-5: LGTM!The theme system is well-structured with:
- Proper CSS variable mapping through
@theme inline- OKLCH color space for better color perception
- Comprehensive light and dark theme definitions
- Appropriate base layer utilities
The custom dark variant and tw-animate-css import align with modern Tailwind CSS practices.
Also applies to: 12-49, 51-66, 69-120, 122-129
frontend/src/features/wikibase-schema/composables/useValueMapping.ts (1)
5-5: LGTM!The migration from string-based icons to Lucide component objects is consistent with the PR's overall pattern. The icon choices are semantically appropriate:
LucideDatabasefor column data sourcesLucideLockfor constant valuesLucideCodefor expressionsConsumers correctly render these using
<component :is="option.icon" />(as seen in ValueMappingEditor.vue).Also applies to: 26-28
frontend/src/features/wikibase-schema/components/ValueMappingEditor.vue (1)
137-148: LGTM!The Button implementation correctly:
- Uses the new
variantprop API (defaultvssecondary)- Renders icons as dynamic components via
<component :is="option.icon" />- Includes both icon and label text for accessibility
- Uses the standardized
smsize valueThe pattern is consistent with the broader shadcn Button migration.
frontend/vite.config.ts (1)
90-92: Thebiomelintrcproperty is correctly used and fully supported.unplugin-auto-import includes a biomelintrc option (with enabled and filepath settings) to generate a .biomelintrc-auto-import.json for Biome integration. The property name change from
eslintrctobiomelintrcis valid and intentional for Biome support. The configuration will be properly recognized and processed.frontend/src/features/wikibase-schema/components/PropertyValueList.vue (1)
2-2: LGTM!The Button migration is correct:
LucidePlusicon semantically matches the add action, variant and size props are appropriate, and the slot-based content is properly structured.Also applies to: 48-56
frontend/src/features/wikibase-schema/components/ReferenceContainer.vue (1)
2-2: LGTM!The Button migration is well-executed: icon choices are semantically correct (
LucidePlusfor add,LucideTrashfor remove), variant selections are appropriate (secondaryfor add,destructivefor remove), and the slot-based structure is consistent.Also applies to: 40-56
frontend/src/features/wikibase-schema/components/TermSection.vue (1)
2-2: LGTM!The Button migration is correct:
LucideXicon is appropriate for the remove action,variant="destructive"semantically matches the action, and the slot-based content is properly structured.Also applies to: 174-181
frontend/components.json (1)
1-21: LGTM!The shadcn-vue configuration is well-structured: the icon library correctly specifies
lucide(matching the Lucide icons used throughout the PR), path aliases align with the TypeScript configuration, and the Tailwind settings (CSS variables, neutral base color, new-york style) provide a solid foundation for the UI migration.frontend/components.d.ts (1)
13-13: LGTM! Button type declaration updated correctly.The global component type reference has been successfully migrated from PrimeVue's Button to the local custom implementation.
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (1)
2-2: LGTM! Lucide icon import added correctly.The import of
LucideChevronDownaligns with the project-wide migration to Lucide icons.frontend/.biomelintrc-auto-import.json (1)
1-426: LGTM! Auto-import configuration looks comprehensive.The new Biome-based auto-import configuration correctly includes the migrated Button-related globals (
Button,ButtonVariants,buttonVariants,cn) alongside existing project globals. The structure is well-formed and aligns with the broader migration from ESLint to Biome tooling.frontend/src/features/data-processing/components/ReplaceDialog.vue (2)
2-2: LGTM! Lucide icons imported correctly.The import of
LucideReplaceandLucideXfollows the project-wide pattern for icon migration.
150-166: LGTM! Dialog footer buttons migrated correctly.Both Cancel and Replace buttons have been successfully migrated to the slot-based icon approach while preserving all functional behavior (disabled states, loading state, click handlers). The icon choices (
LucideXfor Cancel,LucideReplacefor Replace) are semantically appropriate.frontend/src/features/wikibase-schema/components/ValidationDisplay.vue (2)
2-2: LGTM! Lucide icons imported correctly.The import of
LucideTrashandLucideXfollows the established migration pattern.
244-252: LGTM! All validation display buttons migrated consistently.All four button instances (error dismiss, warning dismiss, and two clear-all buttons) have been successfully migrated to the slot-based icon approach with consistent
size="sm"andvariant="secondary"styling. The icon choices are semantically appropriate and all functional behavior is preserved.Also applies to: 276-284, 294-301, 340-349
frontend/src/features/wikibase-schema/components/ClaimEditor.vue (2)
2-2: LGTM! Lucide icons imported correctly.The import of statement rank icons (
LucideStar,LucideCircle,LucideCircleX) follows the established pattern.
22-26: LGTM! Rank selection buttons migrated elegantly.The migration uses a clean pattern where Lucide icon components are stored in the
rankOptionsdata structure and rendered dynamically via<component :is="option.icon" />. The variant logic correctly highlights the selected rank, and the icon choices are semantically appropriate (Star for preferred, Circle for normal, CircleX for deprecated).Also applies to: 52-63
frontend/src/components/ui/button/index.ts (2)
1-4: LGTM! Button component exports set up correctly.The imports from
class-variance-authorityand the Button component export follow best practices for a variant-driven component system.
36-36: LGTM! ButtonVariants type export is correct.The type export using
VariantPropscorrectly infers the prop types from thebuttonVariantsdefinition, enabling type-safe usage across components.frontend/src/features/wikibase-schema/components/SchemaSelector.vue (1)
125-128: LGTM: slot-based Button migration is correct.Icons render via slots, variants/sizes align with buttonVariants, and labels ensure accessibility.
Also applies to: 172-180
frontend/src/features/wikibase-schema/components/ReferencesEditor.vue (1)
62-69: LGTM: add actions migrated cleanly.Secondary + sm with icon and text reads well and remains accessible.
Also applies to: 99-106
frontend/src/features/wikibase-schema/components/StatementEditor.vue (3)
169-175: LGTM: “Add New Claim” migration.Outline variant with icon+label is consistent with the new Button API.
185-191: LGTM: New statement actions.Clear labels, proper disabled state on Save; looks good.
Also applies to: 194-197
121-129: ****The method
removeStatement1exists in the store (line 386 of schema.store.ts) and will not throw at runtime. BothremoveStatementandremoveStatement1are intentionally defined and exported methods—they serve different purposes:removeStatementmodifies both thestatementsarray andstatements1dict, whileremoveStatement1only modifiesstatements1. The call at line 126 is valid.Likely an incorrect or invalid review comment.
frontend/src/features/project-management/pages/OpenProject.vue (1)
64-65: LGTM: empty state and create actions.Lucide icon usage and slot-based buttons look consistent.
Also applies to: 69-72
frontend/auto-imports.d.ts (1)
489-490: The review comment is based on incorrect assumptions about auto-generated files.Button is already registered in
components.d.tsline 13 as part of theGlobalComponentsinterface. Bothauto-imports.d.tsandcomponents.d.tsare auto-generated by unplugin-auto-import and unplugin-vue-components respectively, as confirmed by their file headers and the vite.config.ts configuration which setsdts: truefor both plugins.The empty
GlobalComponentsinterface inauto-imports.d.tsis a separate module augmentation fromunplugin-auto-import. Manual edits to auto-generated files will be overwritten on the next build. The concern is invalid because:
- Button is already properly type-available in templates via
components.d.ts- Button is also available globally in scripts (via
declare global)- TypeScript correctly merges the two separate
declare module 'vue'blocksNo changes are needed to the code.
Likely an incorrect or invalid review comment.
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue (5)
201-209: Icon-only button with aria-label is acceptable.The back button uses
size="icon"with only an icon andaria-label. This is acceptable for accessibility, though visually-hidden text is sometimes preferred for screen readers.
245-264: Button migrations look good.The Add item and Preview buttons correctly use the new Button API with
variant="outline",size="sm", inline Lucide icons, and descriptive text labels.
278-286: Help button migration looks good.The Help button correctly uses the new Button API with
variant="outline",size="sm", inline Lucide icon, and descriptive text.
311-314: Empty state button looks good.The button correctly uses slot-based content with icon and text. Missing explicit variant/size props is acceptable as defaults will apply.
329-339: Item action buttons look good.Both Edit and Delete buttons correctly use the new Button API with appropriate sizes, the destructive variant for delete, inline Lucide icons, and descriptive text labels.
| --destructive: oklch(0.577 0.245 27.325); | ||
| --destructive-foreground: oklch(0.577 0.245 27.325); |
There was a problem hiding this comment.
Fix destructive color contrast issue.
The --destructive and --destructive-foreground variables are set to the same OKLCH value oklch(0.577 0.245 27.325), which would render text invisible on destructive-colored backgrounds in light mode. The foreground should be a contrasting color (typically white or very light).
Apply this diff to fix the contrast:
--destructive: oklch(0.577 0.245 27.325);
- --destructive-foreground: oklch(0.577 0.245 27.325);
+ --destructive-foreground: oklch(0.985 0 0);Note: The dark theme (lines 102-103) correctly uses different values for background and foreground.
📝 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.
| --destructive: oklch(0.577 0.245 27.325); | |
| --destructive-foreground: oklch(0.577 0.245 27.325); | |
| --destructive: oklch(0.577 0.245 27.325); | |
| --destructive-foreground: oklch(0.985 0 0); |
🤖 Prompt for AI Agents
In frontend/src/assets/base.css around lines 67 to 68, the --destructive and
--destructive-foreground variables are currently identical, making destructive
text invisible on destructive-colored backgrounds in light mode; update
--destructive-foreground to a high-contrast, very light value (e.g., a
near-white OKLCH or plain white) so text is readable on the destructive
background, and ensure this mirrors the intent of the dark-theme pair on lines
~102-103 (background darker, foreground light).
| variant="destructive" | ||
| size="sm" | ||
| class="p-button-sm delete-button" | ||
| @click="deleteProject(data)" | ||
| /> | ||
| > | ||
| <LucideTrash /> | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
Remove legacy PrimeVue props/classes; add an accessible name.
icon="pi pi-trash" and p-button-sm are leftovers and ineffective with the new Button.
-<Button
- icon="pi pi-trash"
- variant="destructive"
- size="sm"
- class="p-button-sm delete-button"
- @click="deleteProject(data)"
->
- <LucideTrash />
-</Button>
+<Button
+ aria-label="Delete project"
+ variant="destructive"
+ size="icon-sm"
+ class="delete-button"
+ @click="deleteProject(data)"
+>
+ <LucideTrash />
+ <span class="sr-only">Delete project</span>
+</Button>Optional: grep for other leftover icon="pi or p-button- classes:
🏁 Script executed:
#!/bin/bash
rg -nP 'icon\s*=\s*"pi ' -g 'frontend/**/*.vue' -C2
rg -nP '\bp-button-(sm|lg|outlined)\b' -g 'frontend/**/*.vue' -C2Length of output: 3092
Verified: Remove legacy PrimeVue props and add accessible name to the delete button.
The script confirms the target file still contains both icon="pi pi-trash" (line 138) and class="p-button-sm" (line 141). The suggested refactor is correct—these props are indeed leftovers from PrimeVue and ineffective with the new Button component. The diff properly:
- Removes the unused
iconprop - Changes
size="sm"tosize="icon-sm"(appropriate for icon-only buttons) - Removes the legacy
p-button-smclass - Adds
aria-label="Delete project"for screen readers - Adds
<span class="sr-only">as an additional accessibility layer
The broader grep also identified 4 other instances of icon="pi " in ValueMappingEditor.vue and TermsEditor.vue that should be audited for similar legacy prop cleanup.
🤖 Prompt for AI Agents
In frontend/src/features/project-management/pages/OpenProject.vue around lines
139 to 146, the delete Button still uses legacy PrimeVue props and classes
(icon="pi pi-trash" and class="p-button-sm") and lacks an accessible name;
remove the unused icon prop and the legacy class, change size="sm" to
size="icon-sm" for an icon-only button, add aria-label="Delete project" and
include a visually hidden <span class="sr-only">Delete project</span> inside the
button; also audit ValueMappingEditor.vue and TermsEditor.vue for remaining
icon="pi ..." occurrences and apply the same cleanup and accessibility addition
where applicable.
| <Button | ||
| icon="pi pi-trash" | ||
| severity="danger" | ||
| size="small" | ||
| text | ||
| variant="destructive" | ||
| size="sm" | ||
| @click="removeQualifier(qualifier.id)" | ||
| /> | ||
| > | ||
| <LucideTrash /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible label to icon-only button.
The remove button renders only an icon without visible text, which creates an accessibility issue for screen reader users. Icon-only buttons should include an aria-label attribute or visually-hidden text to describe the action.
Apply this diff to add an accessible label:
<Button
variant="destructive"
size="sm"
+ aria-label="Remove qualifier"
@click="removeQualifier(qualifier.id)"
>
<LucideTrash />
</Button>📝 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.
| <Button | |
| icon="pi pi-trash" | |
| severity="danger" | |
| size="small" | |
| text | |
| variant="destructive" | |
| size="sm" | |
| @click="removeQualifier(qualifier.id)" | |
| /> | |
| > | |
| <LucideTrash /> | |
| </Button> | |
| <Button | |
| variant="destructive" | |
| size="sm" | |
| aria-label="Remove qualifier" | |
| @click="removeQualifier(qualifier.id)" | |
| > | |
| <LucideTrash /> | |
| </Button> |
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/components/QualifiersEditor.vue around
lines 65 to 71 the icon-only remove button lacks an accessible label; update the
Button to include an aria-label (for example aria-label="Remove qualifier") or
add visually-hidden text describing the action (optionally including the
qualifier id or name for context), ensuring the label is localized if the app
uses i18n so screen readers can announce the purpose of the button.
| variant="destructive" | ||
| size="sm" | ||
| @click="removeReference(reference.id)" | ||
| /> | ||
| > | ||
| <LucideTrash /> | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
Add accessible names to icon‑only destructive buttons; consider icon‑sm size.
Icon-only buttons should expose an accessible label. Also, use the square icon size for consistency.
Apply one of these patterns:
- Add aria-label:
- <Button
+ <Button
+ aria-label="Delete reference"
variant="destructive"
- size="sm"
+ size="icon-sm"
@click="removeReference(reference.id)"
>
<LucideTrash />
</Button>- <Button
+ <Button
+ aria-label="Remove snak"
variant="destructive"
- size="sm"
+ size="icon-sm"
@click="removeSnakFromReference(reference.id, snak.id)"
>
<LucideX />
</Button>- Or add visually hidden text:
<Button variant="destructive" size="icon-sm" @click="removeReference(reference.id)">
<LucideTrash />
+ <span class="sr-only">Delete reference</span>
</Button>Also applies to: 117-123
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/components/ReferencesEditor.vue around
lines 83-89 and 117-123, the icon-only destructive buttons lack accessible names
and should use the square icon size for consistency; update each Button to
either include an aria-label (e.g., aria-label="Remove reference") or add
visually hidden text inside the button for screen readers, and change the Lucide
icon to the square "icon-sm" sizing so the icon is consistently sized and
accessible.
| <Button | ||
| v-if="canMoveUp" | ||
| v-tooltip="'Move up'" | ||
| icon="pi pi-chevron-up" | ||
| size="small" | ||
| severity="secondary" | ||
| text | ||
| size="sm" | ||
| variant="secondary" | ||
| @click="$emit('move-up')" | ||
| /> | ||
| > | ||
| <LucideChevronUp /> | ||
| </Button> | ||
|
|
There was a problem hiding this comment.
Add accessible names to icon‑only buttons; use icon‑sm size.
Improve a11y and visual consistency for move/remove controls.
-<Button v-if="canMoveUp" v-tooltip="'Move up'" size="sm" variant="secondary" @click="$emit('move-up')">
+<Button v-if="canMoveUp" aria-label="Move up" size="icon-sm" variant="secondary" @click="$emit('move-up')">
<LucideChevronUp />
</Button>
-<Button v-if="canMoveDown" v-tooltip="'Move down'" size="sm" variant="secondary" @click="$emit('move-down')">
+<Button v-if="canMoveDown" aria-label="Move down" size="icon-sm" variant="secondary" @click="$emit('move-down')">
<LucideChevronDown />
</Button>
-<Button v-tooltip="'Remove statement'" size="sm" variant="destructive" @click="schemaStore.removeStatement(statement.id)">
+<Button aria-label="Remove statement" size="icon-sm" variant="destructive" @click="schemaStore.removeStatement(statement.id)">
<LucideTrash />
</Button>Also applies to: 111-120, 123-130
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue
Show resolved
Hide resolved
| export function cn(...inputs: ClassValue[]) { | ||
| return twMerge(clsx(inputs)) | ||
| } |
There was a problem hiding this comment.
Fix the argument spreading in the cn function.
The clsx function expects variadic arguments, but you're passing the entire inputs array as a single argument. This will not correctly process the class values.
Apply this diff:
export function cn(...inputs: ClassValue[]) {
- return twMerge(clsx(inputs))
+ return twMerge(clsx(...inputs))
}🤖 Prompt for AI Agents
In frontend/src/lib/utils.ts around lines 5 to 7, the cn function passes the
inputs array as a single argument to clsx which expects variadic arguments;
change the call to spread the inputs into clsx (i.e., use clsx(...inputs)) so
clsx processes each ClassValue correctly, then pass the resulting string to
twMerge as before.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/features/project-management/pages/OpenProject.vue (1)
144-151: Complete the delete button migration and add accessibility.The previous review's suggestions were only partially applied. The button still has the legacy
p-button-smclass, usessize="sm"instead ofsize="icon-sm"for an icon-only button, and lacks accessibility attributes.Apply this diff to complete the migration:
<Button + aria-label="Delete project" variant="destructive" - size="sm" - class="p-button-sm delete-button" + size="icon-sm" + class="delete-button" @click="deleteProject(data)" > <LucideTrash /> + <span class="sr-only">Delete project</span> </Button>
🧹 Nitpick comments (2)
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (1)
169-180: TypeScript error resolved; consider more precise typing or cleaner handler.The explicit
Eventtype fixes the pipeline error from the previous review. The Button migration to slot-based icon content is correct.For improved type precision or cleaner code, consider one of these alternatives:
Option 1: Use the more specific
MouseEventtype:- @click="(event: Event) => menu.toggle(event)" + @click="(event: MouseEvent) => menu.toggle(event)"Option 2: Pass the method reference directly (cleanest):
- @click="(event: Event) => menu.toggle(event)" + @click="menu.toggle"Minor formatting nit (Line 179):
- <LucideChevronDown /> + <LucideChevronDown />frontend/src/features/project-management/pages/OpenProject.vue (1)
110-110: Migrate the remaining PrimeVue icon to Lucide.This PrimeVue icon (
pi pi-file) should be replaced with a Lucide icon for consistency with the rest of the migration.Consider using
LucideFileorLucideFileText:+import { ..., LucideFile } from 'lucide-vue-next'Then replace the icon:
-<i class="pi pi-file text-lg text-blue-600" /> +<LucideFile class="text-blue-600" :size="18" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue(2 hunks)frontend/src/features/project-management/pages/OpenProject.vue(4 hunks)frontend/src/features/wikibase-schema/components/DropZone.vue(2 hunks)frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/features/wikibase-schema/components/DropZone.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
frontend/src/features/data-processing/components/ColumnHeaderMenu.vue (1)
2-2: LGTM!The Lucide icon import is correct and properly used in the template.
frontend/src/features/project-management/pages/OpenProject.vue (5)
2-8: Clean icon imports for the migration.All necessary Lucide icons are imported to replace PrimeVue icons throughout the component.
39-47: Well done addressing the previous loading prop issue!The refresh button now correctly uses
:disabled="isLoading"with conditional icon rendering (animated spinner vs. refresh icon) and dynamic text. This properly replaces the unsupportedloadingprop.
49-52: Proper slot-based Button implementation.The create button correctly uses the new Button component's slot pattern with the Lucide icon.
69-71: Good empty state improvements.The addition of
flex-colensures proper vertical layout, and the explicit:size="32"prop provides consistent icon sizing.
76-79: Consistent Button implementation.Matches the pattern used for the header create button—clean and consistent.
frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue (7)
2-13: LGTM! All imported Lucide icons are used.The icon imports are clean and all are utilized in the template for the button migration.
196-204: LGTM! Icon button migration is correct.The back button properly uses
size="icon"for icon-only buttons and includes anaria-labelfor accessibility.
240-249: LGTM! Toolbar buttons are properly migrated.The Add item, Preview, and Help buttons consistently use
variant="outline"andsize="sm"with Lucide icons rendered in the button slots alongside text labels. The migration is clean and complete.Also applies to: 250-259, 273-281
260-272: Save button migration successfully addresses past concerns.The save button now properly uses the shadcn Button API:
- Removed PrimeVue-specific props (
:icon,:severity,:loading)- Conditional icon rendering in the slot based on state
- Dynamic variant via
:variantprop- Proper loading state with
LucideLoader2andanimate-spinclassThis resolves the incomplete migration flagged in the previous review.
306-309: LGTM! Empty state button uses default styling appropriately.The button correctly uses the default variant (no explicit
variantprop), which is appropriate for the primary action in an empty state.
324-327: Verify the missing variant prop on Edit button is intentional.The Edit button at line 324 doesn't specify a
variantprop, so it will use the default variant. This differs from the toolbar buttons (Add item, Preview, Help) which usevariant="outline".Please confirm whether this is intentional based on the different context (item actions vs. toolbar actions), or if it should be consistent with other secondary actions.
328-335: LGTM! Delete button properly uses destructive variant.The delete button correctly uses
variant="destructive"for a destructive action, with appropriate icon and text labels.
| const getSaveButtonSeverity = () => { | ||
| if (saveStatus.value === 'success' && !schemaStore.isDirty) return 'success' | ||
| if (saveStatus.value === 'error') return 'danger' | ||
| return undefined | ||
| if (saveStatus.value === 'success' && !schemaStore.isDirty) return 'default' | ||
| if (saveStatus.value === 'error') return 'destructive' | ||
| return 'outline' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename function to match its new purpose.
The function name getSaveButtonSeverity() is misleading since it now returns shadcn Button variant values ('default', 'destructive', 'outline') rather than PrimeVue severity values.
Apply this diff to rename the function:
-const getSaveButtonSeverity = () => {
+const getSaveButtonVariant = () => {
if (saveStatus.value === 'success' && !schemaStore.isDirty) return 'default'
if (saveStatus.value === 'error') return 'destructive'
return 'outline'
}And update the call site at line 262:
<Button
data-testid="save-btn"
- :variant="getSaveButtonSeverity()"
+ :variant="getSaveButtonVariant()"
:disabled="!canSave || isSaving"
size="sm"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/features/wikibase-schema/components/WikibaseSchemaEditor.vue
around lines 159 to 163, the function getSaveButtonSeverity() now returns shadcn
Button variant strings but its name is misleading; rename it to
getSaveButtonVariant and update the call site at line 262 to use
getSaveButtonVariant(); keep the same return values and behavior, and ensure any
other references to the old name in this file are updated to the new name.
No description provided.