Conversation
fix(storybook): resolve typing and script issues
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR removes three internal documentation markdown files from the repository. While titled "🚀 prod", the changes consist entirely of deleting documentation files that are not referenced elsewhere in the codebase.
Key changes:
- Complete removal of internal/working documentation files
- No code or functional changes
- No impact on the published package or user-facing documentation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| DATATABLE.md | Removes 536 lines of DataTable component documentation (appears to be internal/draft documentation not referenced in the main README) |
| COMPONENTS_EXAMPLES.md | Removes 360 lines of component usage examples (internal documentation not linked from the main README) |
| AUDIT_REPORT.md | Removes 2,251 lines of detailed audit report and best practices analysis (internal report dated 2025-12-14, not part of public documentation) |
Note: These files are not referenced anywhere in the codebase (README.md, package.json, or other files), suggesting they were internal working documents or drafts that are being cleaned up before production deployment.
Comments suppressed due to low confidence (1)
DATATABLE.md:1
- The PR title "🚀 prod" is vague and doesn't describe what changes are being made. Consider using a more descriptive title that clearly indicates documentation files are being removed, such as "docs: remove unused documentation files" or "chore: clean up internal documentation (DATATABLE.md, COMPONENTS_EXAMPLES.md, AUDIT_REPORT.md)". This helps with project history and makes it easier to understand what was changed when reviewing the commit log.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <template> | ||
| <!-- Root element con classe dal theme --> |
There was a problem hiding this comment.
There's duplicate template code on lines 67-68. Line 67 contains a complete template closing tag </template> followed by line 68 which starts another template section. This will cause a syntax error. One of these template tags should be removed.
| */ | ||
|
|
||
| import { computed } from 'vue' | ||
| import componentTheme from '@/themes/button' // Cambia con il tuo theme |
There was a problem hiding this comment.
The import statement on line 13 references '@/themes/button' but the comment says to change it to your own theme. Since this is a template file, this import will fail for anyone copying this template unless they happen to have a button theme. Consider using a more generic placeholder like '@/themes/component' or adding a comment to explicitly replace this.
| import componentTheme from '@/themes/button' // Cambia con il tuo theme | |
| import componentTheme from '@/themes/component' // Cambia con il tuo theme |
| // // Mock router-link for Storybook | ||
| // setup((app) => { | ||
| // app.component('router-link', { | ||
| // props: ['to'], | ||
| // template: '<a :href="to"><slot /></a>' | ||
| // }) | ||
| // }) |
There was a problem hiding this comment.
Storybook setup is commented out, which will break any stories that rely on router-link components. If router-link mocking is no longer needed, this code should be removed entirely rather than commented out. If it's still needed, it should be uncommented.
| }) | ||
|
|
||
| const emit = defineEmits(['update:show']) | ||
| const emit = defineEmits<{ 'update:show': [] }>() |
There was a problem hiding this comment.
The emit definition for 'update:show' is missing the payload type. If this emit doesn't send any payload, it should be defined as an empty array with no type parameters. However, this is unusual for an 'update:show' event which typically passes a boolean value. Consider using 'update:show': [value: boolean] instead.
| const emit = defineEmits<{ 'update:show': [] }>() | |
| const emit = defineEmits<{ 'update:show': [value: boolean] }>() |
| const slots = defineSlots() | ||
|
|
||
| const emits = defineEmits(['close']) | ||
| const emits = defineEmits<{ close: [] }>() |
There was a problem hiding this comment.
The emit definition for 'close' is missing the payload type. If the event sends no data, the empty array is correct, but typically close events may include an Event object. Consider if you need close: [event?: MouseEvent] for better type safety.
| 'row:click': [any] | ||
| 'row:hover': [any] | ||
| 'row:select': [any] |
There was a problem hiding this comment.
The DataTable emits have inconsistent tuple syntax. Lines 88-94 use square bracket syntax [SortingState] while lines 95-97 use [any]. For consistency and better type safety, all emits should use the same pattern and 'any' should be replaced with specific types like [row: any].
| 'row:click': [any] | |
| 'row:hover': [any] | |
| 'row:select': [any] | |
| 'row:click': [row: any] | |
| 'row:hover': [row: any] | |
| 'row:select': [row: any] |
| const handleRowClick = (row: any, event: MouseEvent | KeyboardEvent) => { | ||
| const target = event.target as HTMLElement | ||
| if (target.closest('button') || target.closest('a')) return | ||
| emit('row:click', row.original) | ||
| props.onSelect?.(row.original) | ||
| if (props.enableRowSelection) { | ||
| row.toggleSelected() | ||
| emit('row:select', row.original) | ||
| } | ||
| } |
There was a problem hiding this comment.
The handleRowClick function signature changed to accept an event parameter, but the event is being passed from template using $event. The event parameter type is MouseEvent | KeyboardEvent, but the function accesses event.target assuming it exists. This could be improved by adding a type guard or properly handling both event types.
|
|
||
| **Current**: | ||
| ```vue | ||
| <div v-else-if="tab.content" class="p-4" v-html="tab.content" /> |
There was a problem hiding this comment.
The use of v-html="tab.content" directly renders arbitrary HTML content and creates a clear XSS risk if tab.content can contain or be influenced by user-generated input. An attacker could inject <script> tags or malicious event handlers into tab.content, which would execute in the context of your application and compromise user data or sessions. To mitigate this, avoid using v-html for untrusted data by switching to safe text rendering or, if HTML is strictly required, enforce robust server-side or well-reviewed client-side sanitization and constrain the allowed markup to a minimal, safe subset.
| <div v-else-if="tab.content" class="p-4" v-html="tab.content" /> | |
| <div v-else-if="tab.content" class="p-4" v-html="sanitizeHtml(tab.content)" /> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 88 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/components/_template.vue:70
- This file currently contains two blocks (a new button example at the top and an older template starting again below). Vue SFCs can only have one root ; the extra block will break compilation. Remove the leftover second template (or convert it to commented docs) and ensure the script/template sections are properly closed and aligned.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| w-auto | ||
| rounded-lg | ||
| bg-white | ||
| px-4 py-3 | ||
| text-gray-900 | ||
| shadow-lg | ||
| border border-gray-200 |
There was a problem hiding this comment.
content/arrow use hard-coded light theme colors (bg-white, text-gray-900, border-gray-200). This will ignore the design tokens used elsewhere (e.g., bg-popover, text-popover-foreground, border-border) and will likely render incorrectly in dark mode. Consider switching back to token-based classes and/or adding dark: equivalents so the popover respects theming.
| color?: 'info' | 'success' | 'warning' | 'error' | 'neutral' | ||
| variant?: 'soft' | 'solid' | 'outline' | 'subtle' | ||
| size?: 'sm' | 'md' | 'lg' |
There was a problem hiding this comment.
AlertProps.color doesn’t include 'primary', but the theme and Storybook stories use color="primary". This will cause type errors and prevents consumers from using the primary tone. Add 'primary' to the color union (or remove primary from the theme/stories if it’s not supported).
| variant: props.variant, | ||
| color: props.color, | ||
| focused: isFocused.value, | ||
| disabled: props.disabled, |
There was a problem hiding this comment.
The theme supports a readonly variant, but the computed theme props passed to inputTheme(...) don't include readonly. As a result, readonly inputs won't get the expected styling/cursor behavior. Pass readonly: props.readonly into the theme call (and consider also exposing a loading prop if you want to use the theme’s loading variant).
| disabled: props.disabled, | |
| disabled: props.disabled, | |
| readonly: props.readonly, |
| size: { | ||
| control: 'select', | ||
| options: ['xs', 'sm', 'md', 'lg', 'xl'], | ||
| description: 'Size of the alert' |
There was a problem hiding this comment.
The story exposes size options xs/xl, but Alert only supports size?: 'sm' | 'md' | 'lg' (and the theme defines the same). This will produce invalid controls/props and likely TS errors. Either extend the component/theme to support xs and xl, or constrain the story options to the supported sizes.
| size: { | ||
| control: 'select', | ||
| options: ['sm', 'md', 'lg'] | ||
| options: ['xs', 'sm', 'md', 'lg', 'xl'] | ||
| }, |
There was a problem hiding this comment.
The story’s size argTypes include xs and xl, but Progress only supports size?: 'sm' | 'md' | 'lg' (and the theme only defines sm/md/lg). This will surface invalid controls and can cause TS/runtime issues. Adjust the story options (or add xs/xl support to the component/theme).
| size: { | ||
| control: 'select', | ||
| options: ['sm', 'md', 'lg'], | ||
| description: 'Button size (sm=small, md=medium, lg=large)' | ||
| options: ['xs', 'sm', 'md', 'lg', 'xl'], | ||
| description: 'Button size' |
There was a problem hiding this comment.
The story exposes size options xs/xl and uses them in the Sizes story, but Button (and src/themes/button.ts) only define sizes sm/md/lg. This will result in invalid props and/or missing styling. Either add xs/xl to the component/theme or limit the story controls/examples to supported sizes.
| size?: 'xs' | 'sm' | 'md' | 'lg' | 'xl' | ||
| color?: 'primary' | 'success' | 'warning' | 'error' | ||
| label?: string |
There was a problem hiding this comment.
SwitchProps.color is still limited to 'primary' | 'success' | 'warning' | 'error', but src/themes/switch.ts now defines additional colors (e.g. secondary, info). This creates a mismatch where theme capabilities can’t be used through the component API and will cause TS friction. Update the prop union to match the theme (or remove the extra theme colors).
No description provided.