-
Notifications
You must be signed in to change notification settings - Fork 0
Implement UI components from @object-ui/types #28
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
Conversation
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
|
@copilot 继续 |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
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.
Pull request overview
This pull request implements the integration of @object-ui/types into @object-ui/components, establishing a centralized type system for all 60+ UI component renderers. This aligns with Object UI's "Schema First" architecture principle where types define the protocol layer.
Changes:
- Added
@object-ui/typesas a workspace dependency to the components package - Updated all component renderers to import and use TypeScript interfaces from the centralized types package
- Fixed property naming inconsistencies (e.g.,
items→optionsin RadioGroup,maxLength→lengthin InputOTP,accordionType→typein Accordion) - Replaced local type definitions with imports from the centralized types package
Reviewed changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/components/package.json | Added @object-ui/types workspace dependency |
| pnpm-lock.yaml | Updated lockfile with new dependency reference |
| packages/components/src/renderers/form/*.tsx | Updated 14 form components to use typed schemas (Button, Input, Textarea, Select, etc.) |
| packages/components/src/renderers/layout/*.tsx | Updated 5 layout components (Card, Container, Flex, Grid, Tabs) |
| packages/components/src/renderers/basic/*.tsx | Updated 6 basic components (Div, Text, Span, Image, Icon, Separator) |
| packages/components/src/renderers/data-display/*.tsx | Updated 7 data display components (Alert, Badge, Avatar, List, etc.) |
| packages/components/src/renderers/feedback/*.tsx | Updated 4 feedback components (Loading, Progress, Skeleton, Toaster) |
| packages/components/src/renderers/disclosure/*.tsx | Updated 2 disclosure components with corrected property names |
| packages/components/src/renderers/overlay/*.tsx | Updated 9 overlay components (Dialog, Sheet, Popover, etc.) |
| packages/components/src/renderers/navigation/*.tsx | Updated 2 navigation components (HeaderBar, Sidebar) |
| packages/components/src/renderers/complex/*.tsx | Updated 11 complex components (Kanban, DataTable, Calendar, etc.) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| {...props} | ||
| > | ||
| {schema.label || renderChildren(schema.body)} | ||
| {schema.label || renderChildren(schema.body || schema.children)} |
Copilot
AI
Jan 14, 2026
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.
The fallback logic schema.body || schema.children suggests uncertainty about the property name. According to the ToggleSchema interface, the property is named children. Consider removing the fallback to schema.body for clarity and type safety, or verify if this is intentionally supporting both property names for backward compatibility.
| @@ -1,4 +1,5 @@ | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { ContextMenuSchema } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
Missing type annotation for component props. The type import was added but the component function parameters are not typed. Add type annotation: ({ schema, className, ...props }: { schema: ContextMenuSchema; className?: string; [key: string]: any }) to ensure type safety and consistency with the pattern used in other components in this PR.
| @@ -1,4 +1,5 @@ | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { SidebarSchema } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
Missing type annotation for component props. The type import was added but the component function parameters are not typed. Although multiple sidebar-related components are registered in this file, at minimum the main 'sidebar' component should have type annotation: ({ schema, ...props }: { schema: SidebarSchema; [key: string]: any }) to ensure type safety.
| @@ -1,5 +1,6 @@ | |||
| // table.tsx implementation | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { TableSchema } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
Missing type annotation for component props. The type import was added but the component function parameters are not typed. Add type annotation: ({ schema, className, ...props }: { schema: TableSchema; className?: string; [key: string]: any }) to ensure type safety and consistency with the pattern used in other components in this PR.
| selected={schema.value || schema.defaultValue} | ||
| className={className} | ||
| {...props} | ||
| {...props as any} |
Copilot
AI
Jan 14, 2026
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.
The use of as any type assertion bypasses TypeScript type checking and eliminates type safety benefits. This suggests a type mismatch between the Calendar component's expected props and the schema properties being passed. Consider fixing the underlying type incompatibility instead of using a type assertion.
| <Accordion type={schema.accordionType || 'single'} collapsible={schema.collapsible} className={className} {...props}> | ||
| {schema.items?.map((item: any, index: number) => ( | ||
| ({ schema, className, ...props }: { schema: AccordionSchema; className?: string; [key: string]: any }) => ( | ||
| <Accordion type={schema.type || 'single'} collapsible={schema.collapsible} className={className} {...props}> |
Copilot
AI
Jan 14, 2026
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.
Incorrect property name used. The schema uses accordionType property according to the type definition in @object-ui/types, but the code references schema.type. This should be schema.accordionType to match the AccordionSchema interface.
| @@ -1,4 +1,5 @@ | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { FormSchema, FormField as FormFieldConfig, ValidationRule, FieldCondition, SelectOption } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
The FormField type is renamed to FormFieldConfig to avoid naming conflict with the FormField component from @/ui/form, which is a good practice. However, the import shows that FormField as FormFieldConfig is imported from @object-ui/types. Verify that the type is actually exported as FormField in the types package, as the export list shows FormField, not FormFieldConfig.
| import type { FilterBuilderSchema, FilterGroup } from '@object-ui/types'; | ||
| import { FilterBuilder } from '@/ui/filter-builder'; |
Copilot
AI
Jan 14, 2026
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.
The import statement is removing the FilterGroup type from being imported from the UI library and instead importing it from @object-ui/types. Verify that FilterGroup is correctly exported from @object-ui/types and that the type definition matches what the FilterBuilder component expects. This is a potential breaking change if the types are not compatible.
| @@ -1,4 +1,5 @@ | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { DropdownMenuSchema } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
Missing type annotation for component props. The type import was added but the component function parameters are not typed. Add type annotation: ({ schema, className, ...props }: { schema: DropdownMenuSchema; className?: string; [key: string]: any }) to ensure type safety and consistency with the pattern used in other components in this PR.
| @@ -1,4 +1,5 @@ | |||
| import { ComponentRegistry } from '@object-ui/core'; | |||
| import type { TimelineSchema } from '@object-ui/types'; | |||
Copilot
AI
Jan 14, 2026
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.
Missing type annotation for component props. The type import was added but the component function parameters are not typed. Add type annotation: ({ schema, className, ...props }: { schema: TimelineSchema; className?: string; [key: string]: any }) to ensure type safety and consistency with the pattern used in other components in this PR.
|
@copilot 帮我修改 |
|
@copilot 继续 |
…mes, remove duplicate types and type assertions Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
|
✅ All checks passed!
|
Implementation Plan
@object-ui/typesas a dependency to@object-ui/componentspackage@object-ui/types:ButtonSchemaInputSchemaTextareaSchemaSelectSchemaCheckboxSchemaRadioGroupSchemaSwitchSchemaToggleSchema✅ Fixed fallback logicSliderSchemaFileUploadSchemaDatePickerSchemaCalendarSchema✅ Removedas anyInputOTPSchemaFormSchema@object-ui/typesCardSchemaContainerSchemaFlexSchemaGridSchemaTabsSchema@object-ui/typesDivSchemaTextSchemaSpanSchemaImageSchemaIconSchemaSeparatorSchema@object-ui/typesAlertSchemaBadgeSchemaAvatarSchemaListSchemaMarkdownSchemaTreeViewSchemaChartSchema@object-ui/typesLoadingSchemaProgressSchemaSkeletonSchemaToasterSchema@object-ui/typesAccordionSchema✅ Fixed property nameCollapsibleSchema@object-ui/typesDialogSchemaAlertDialogSchemaPopoverSchemaTooltipSchemaHoverCardSchemaSheetSchemaDrawerSchemaDropdownMenuSchema✅ Added type annotationContextMenuSchema✅ Added type annotation@object-ui/typesHeaderBarSchemaSidebarSchema✅ Added type annotation@object-ui/typesKanbanSchemaCarouselSchemaCalendarViewSchemaFilterBuilderSchemaChatbotSchemaDataTableSchema✅ Removed duplicate type definitionTableSchema✅ Added type annotationScrollAreaSchemaResizableSchemaTimelineSchema✅ Added type annotationSummary
This PR completes the integration of
@object-ui/typesinto@object-ui/components, ensuring all 60 component renderers use proper TypeScript types from the centralized type definitions package. This improves type safety, maintainability, and follows the "Schema First" architecture principle of Object UI.Code Review Fixes Applied:
schema.body ||fallback in toggle.tsx (now uses onlyschema.children)schema.typetoschema.accordionTypeas anytype assertion in calendar.tsxOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.