Design token Improvements #478
Conversation
|
@kgpatidar is attempting to deploy a commit to the Chai Builder Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the design tokens management system with improved UI/UX, better organization, and enhanced functionality including real-time editing and lazy loading.
Key Changes
- Replaced
FontStyleIconwithTokensIconfor better semantic representation - Restructured design token management into a modular directory structure with separate components
- Enhanced
ManualClassescomponent to support both default and design token contexts with conditional rendering - Implemented lazy loading for the design tokens panel to improve initial bundle size
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/components/sidepanels/panels/design-tokens/DesignTokensIcon.tsx | Updated icon from FontStyleIcon to TokensIcon for better visual representation |
| src/core/components/settings/new-panel/manual-classes.tsx | Extended component to support design token editing mode with new props and conditional logic |
| src/core/components/settings/new-panel/manage-design-tokens.tsx | Deleted old monolithic component |
| src/core/components/settings/new-panel/manage-design-token/manage-design-tokens.tsx | New modular component with real-time editing, search functionality, and improved UI |
| src/core/components/settings/new-panel/manage-design-token/design-token-utils.ts | Extracted validation and utility functions for better code organization |
| src/core/components/settings/new-panel/manage-design-token/design-token-usage.tsx | Refactored to accept children prop for flexible rendering |
| src/core/components/settings/new-panel/manage-design-token/delete-design-token.tsx | New component for delete confirmation dialog with better UX |
| src/core/components/layout/root-layout.tsx | Added lazy loading for ManageDesignTokens component to optimize bundle size |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onAddNew?: any; | ||
| onRemove?: any; |
There was a problem hiding this comment.
The onAddNew prop is typed as any, which disables type safety. Based on the implementation, onAddNew is called with a string array (lines 98 and 277), but the consumer in manage-design-tokens.tsx provides handleAddClass which expects a single string parameter (line 250-252 in manage-design-tokens.tsx). This type mismatch will cause bugs.
The proper fix requires:
- Type
onAddNewcorrectly (e.g.,onAddNew?: (classes: string[]) => void) - Update
handleAddClassin manage-design-tokens.tsx to accept a string array and properly merge all classes using twMerge, or - Modify the calls in manual-classes.tsx to pass a single joined string instead of an array
Similarly, onRemove should be typed as (className: string) => void.
| onAddNew?: any; | |
| onRemove?: any; | |
| onAddNew?: (classes: string[]) => void; | |
| onRemove?: (className: string) => void; |
| import { Button } from "@/ui/shadcn/components/ui/button"; | ||
| import { Tooltip, TooltipContent, TooltipTrigger } from "@/ui/shadcn/components/ui/tooltip"; | ||
| import { CopyIcon, Cross2Icon, PlusIcon } from "@radix-ui/react-icons"; | ||
| import { Label } from "@radix-ui/react-dropdown-menu"; |
There was a problem hiding this comment.
The Label component is being imported from "@radix-ui/react-dropdown-menu", which is incorrect. Based on the codebase conventions (as seen in src/core/components/canvas/topbar/ai-assistant.tsx:5, src/core/components/settings/new-panel/attributes-editor.tsx, and other files), Label should be imported from "@/ui/shadcn/components/ui/label". The dropdown-menu Label is designed for dropdown menu items, not form labels, and has different semantics and styling.
| import { Label } from "@radix-ui/react-dropdown-menu"; | |
| import { Label } from "@/ui/shadcn/components/ui/label"; |
| const prop = first(styleBlock)?.prop as string; | ||
| const { classes: classesString } = getSplitChaiClasses(get(block, prop, "")); | ||
| const classes = classesString.split(" ").filter((cls) => !isEmpty(cls)); | ||
| const classes = (from === "default" ? classesString : classFromProps).split(" ").filter((cls) => !isEmpty(cls)); |
There was a problem hiding this comment.
When from === "designToken" and classFromProps is undefined, calling .split() on undefined will cause a runtime error. The component should handle the case where classFromProps might be undefined by providing a default value (e.g., empty string).
| const classes = (from === "default" ? classesString : classFromProps).split(" ").filter((cls) => !isEmpty(cls)); | |
| const classesSource = from === "default" ? classesString : classFromProps ?? ""; | |
| const classes = classesSource.split(" ").filter((cls) => !isEmpty(cls)); |
| onRemove(cls); | ||
| setNewCls(cls); |
There was a problem hiding this comment.
When from === "designToken", onRemove is called without checking if it's defined. If onRemove is undefined, this will cause a runtime error. Add a check to ensure onRemove is a function before calling it, similar to the pattern used with onAddNew on line 98.
| if (from === "default") { | ||
| removeClassesFromBlocks(selectedIds, [cls]); | ||
| } else { | ||
| onRemove(cls); |
There was a problem hiding this comment.
When from === "designToken", onRemove is called without checking if it's defined. If onRemove is undefined, this will cause a runtime error. Add a check to ensure onRemove is a function before calling it, similar to the pattern used with onAddNew on line 98.
1 participant
No description provided.