Extend FormRenderer to support all 28 OpenStack standard field types#180
Merged
Extend FormRenderer to support all 28 OpenStack standard field types#180
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
hotlong
changed the base branch from
copilot/refactor-object-form-component
to
main
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add support for all field types from zod definition
Extend FormRenderer to support all 28 OpenStack standard field types
Jan 24, 2026
github-actions
bot
added
documentation
Contributor
📦 Bundle Size Report
Size Limits
|
Contributor
|
✅ All checks passed!
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the FormRenderer component to support all 28 field types defined in the ObjectStack field type specification, increasing coverage from 11 to 28 types. It introduces an ExtendedFormField interface to handle complex widget properties (file uploads, select options, etc.) that aren't part of the base @objectstack/spec FormField schema.
Changes:
- Added 17 new field widget types including currency, percent, phone, file uploads, location, relationships (lookup/master_detail/user/owner), computed fields (formula/summary/auto_number), and specialized types (object, vector, grid)
- Introduced ExtendedFormField interface with properties for multiple selection, file accept types, and select options
- Added comprehensive test coverage for new field types
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| packages/react/src/components/form/FieldFactory.tsx | Core implementation of 17 new field widget types with validation and specialized input handling |
| packages/react/src/components/form/index.ts | Added ExtendedFormField to public exports |
| packages/react/src/components/form/README.md | Comprehensive documentation update listing all 28 supported widget types with examples |
| packages/react/src/components/form/FormRenderer.test.tsx | Added 14 tests covering new field types (currency, percent, phone, markdown, html, file, image, location, lookup, user, formula, object) |
Comments suppressed due to low confidence (1)
packages/react/src/components/form/FieldFactory.tsx:502
- According to the ObjectUI coding guidelines (Rule #2: "Shadcn Native" Aesthetics), components should strictly follow Shadcn's DOM structure and patterns. The current implementation uses raw HTML inputs rather than Shadcn UI components from @object-ui/components. While this works functionally, it diverges from the architectural principle that ObjectUI is "Serializable Shadcn". Consider either: 1) adding @object-ui/components as a dependency and using the Shadcn Input, Select, Textarea components, or 2) documenting why this package intentionally uses raw HTML (e.g., for portability or to avoid circular dependencies).
export interface FieldFactoryProps {
/**
* Field configuration from FormFieldSchema
*/
field: FormField;
/**
* React Hook Form methods
*/
methods: UseFormReturn<any>;
/**
* Whether the field is disabled
*/
disabled?: boolean;
}
/**
* FieldFactory component that renders different input types based on
* the widget property or field type
*/
export const FieldFactory: React.FC<FieldFactoryProps> = ({
field,
methods,
disabled = false,
}) => {
const { register, formState: { errors } } = methods;
// Cast to extended field for properties not in base schema
const extendedField = field as ExtendedFormField;
// Determine the widget type
const widgetType = field.widget || 'text';
const fieldName = field.field;
const error = errors[fieldName];
// Helper function to handle multiple select value conversion
const handleMultipleSelectValue = (value: any) => {
if (extendedField.multiple && value instanceof HTMLCollection) {
return Array.from(value as HTMLCollectionOf<HTMLOptionElement>).map((opt) => opt.value);
}
return value;
};
// Handle conditional visibility
// Note: visibleOn expression evaluation is not yet implemented
// Fields are always visible unless explicitly hidden
// Skip if explicitly hidden
if (field.hidden) {
return null;
}
// Common field wrapper
const renderField = (input: React.ReactNode) => (
<div className="space-y-2">
{field.label && (
<label htmlFor={fieldName} className="block text-sm font-medium text-gray-700">
{field.label}
{field.required && <span className="text-red-500 ml-1">*</span>}
</label>
)}
{input}
{field.helpText && (
<p className="text-sm text-gray-500">{field.helpText}</p>
)}
{error && (
<p className="text-sm text-red-600">{error.message as string}</p>
)}
</div>
);
// Render based on widget type
switch (widgetType.toLowerCase()) {
case 'text':
case 'string':
case 'email':
case 'password':
case 'url':
case 'tel':
return renderField(
<input
id={fieldName}
type={widgetType === 'string' ? 'text' : widgetType}
placeholder={field.placeholder}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'number':
case 'integer':
case 'float':
return renderField(
<input
id={fieldName}
type="number"
placeholder={field.placeholder}
disabled={disabled}
step={widgetType === 'integer' ? '1' : 'any'}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
valueAsNumber: true,
})}
/>
);
case 'checkbox':
case 'boolean':
return (
<div className="flex items-start space-x-2">
<input
id={fieldName}
type="checkbox"
disabled={disabled}
className="mt-1 h-4 w-4 text-blue-600 border-gray-300 rounded focus:ring-2 focus:ring-blue-500 disabled:opacity-50"
{...register(fieldName)}
/>
<div className="flex-1">
{field.label && (
<label htmlFor={fieldName} className="text-sm font-medium text-gray-700">
{field.label}
{field.required && <span className="text-red-500 ml-1">*</span>}
</label>
)}
{field.helpText && (
<p className="text-sm text-gray-500 mt-1">{field.helpText}</p>
)}
{error && (
<p className="text-sm text-red-600 mt-1">{error.message as string}</p>
)}
</div>
</div>
);
case 'textarea':
return renderField(
<textarea
id={fieldName}
placeholder={field.placeholder}
disabled={disabled}
rows={4}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'select':
case 'dropdown':
return renderField(
<select
id={fieldName}
disabled={disabled}
multiple={extendedField.multiple}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
setValueAs: handleMultipleSelectValue,
})}
>
{!extendedField.multiple && <option value="">{field.placeholder || 'Select an option'}</option>}
{extendedField.options?.map((option) => (
<option key={option.value} value={option.value} disabled={option.disabled}>
{option.label}
</option>
))}
</select>
);
case 'date':
return renderField(
<input
id={fieldName}
type="date"
placeholder={field.placeholder}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'datetime':
case 'datetime-local':
return renderField(
<input
id={fieldName}
type="datetime-local"
placeholder={field.placeholder}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'time':
return renderField(
<input
id={fieldName}
type="time"
placeholder={field.placeholder}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'currency':
return renderField(
<div className="relative">
<span className="absolute left-3 top-2 text-gray-500">$</span>
<input
id={fieldName}
type="number"
placeholder={field.placeholder}
disabled={disabled}
step="0.01"
className="w-full pl-8 pr-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
valueAsNumber: true,
})}
/>
</div>
);
case 'percent':
return renderField(
<div className="relative">
<input
id={fieldName}
type="number"
placeholder={field.placeholder}
disabled={disabled}
step="0.01"
min="0"
max="100"
className="w-full pr-8 pl-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
valueAsNumber: true,
})}
/>
<span className="absolute right-3 top-2 text-gray-500">%</span>
</div>
);
case 'phone':
return renderField(
<input
id={fieldName}
type="tel"
placeholder={field.placeholder || '+1 (555) 000-0000'}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'markdown':
return renderField(
<textarea
id={fieldName}
placeholder={field.placeholder || 'Enter markdown text...'}
disabled={disabled}
rows={8}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed font-mono"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'html':
return renderField(
<textarea
id={fieldName}
placeholder={field.placeholder || 'Enter HTML...'}
disabled={disabled}
rows={8}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed font-mono"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'file':
return renderField(
<input
id={fieldName}
type="file"
disabled={disabled}
multiple={extendedField.multiple}
accept={extendedField.accept?.join(',')}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed file:mr-4 file:py-2 file:px-4 file:rounded-md file:border-0 file:text-sm file:font-semibold file:bg-blue-50 file:text-blue-700 hover:file:bg-blue-100"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'image':
return renderField(
<input
id={fieldName}
type="file"
disabled={disabled}
multiple={extendedField.multiple}
accept={extendedField.accept?.join(',') || 'image/*'}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed file:mr-4 file:py-2 file:px-4 file:rounded-md file:border-0 file:text-sm file:font-semibold file:bg-blue-50 file:text-blue-700 hover:file:bg-blue-100"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
case 'location':
return renderField(
<div className="space-y-2">
<input
id={`${fieldName}-lat`}
type="number"
placeholder="Latitude"
disabled={disabled}
step="any"
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(`${fieldName}.lat`, {
required: field.required ? 'Latitude is required' : false,
valueAsNumber: true,
})}
/>
<input
id={`${fieldName}-lng`}
type="number"
placeholder="Longitude"
disabled={disabled}
step="any"
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(`${fieldName}.lng`, {
required: field.required ? 'Longitude is required' : false,
valueAsNumber: true,
})}
/>
</div>
);
case 'lookup':
case 'master_detail':
return renderField(
<select
id={fieldName}
disabled={disabled}
multiple={extendedField.multiple}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
setValueAs: handleMultipleSelectValue,
})}
>
{!extendedField.multiple && <option value="">{field.placeholder || 'Select an option'}</option>}
{extendedField.options?.map((option) => (
<option key={option.value} value={option.value} disabled={option.disabled}>
{option.label}
</option>
))}
</select>
);
case 'user':
case 'owner':
return renderField(
<select
id={fieldName}
disabled={disabled}
multiple={extendedField.multiple}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
setValueAs: handleMultipleSelectValue,
})}
>
{!extendedField.multiple && <option value="">{field.placeholder || 'Select user'}</option>}
{extendedField.options?.map((option) => (
<option key={option.value} value={option.value} disabled={option.disabled}>
{option.label}
</option>
))}
</select>
);
case 'formula':
case 'summary':
case 'auto_number':
// Read-only computed fields - display as disabled text input
return renderField(
<input
id={fieldName}
type="text"
disabled={true}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm bg-gray-50 cursor-not-allowed text-gray-600"
{...register(fieldName)}
/>
);
case 'object':
return renderField(
<textarea
id={fieldName}
placeholder={field.placeholder || 'Enter JSON object...'}
disabled={disabled}
rows={6}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed font-mono text-sm"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
validate: (value) => {
if (!value) return true;
try {
JSON.parse(value);
return true;
} catch (e) {
return 'Invalid JSON format';
}
},
})}
/>
);
case 'vector':
// Vector fields are typically read-only or require specialized input
return renderField(
<input
id={fieldName}
type="text"
placeholder={field.placeholder || 'Vector data (read-only)'}
disabled={true}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm bg-gray-50 cursor-not-allowed text-gray-600"
{...register(fieldName)}
/>
);
case 'grid':
// Grid fields require complex table/grid editor - placeholder for now
return renderField(
<div className="w-full px-3 py-2 border border-gray-300 rounded-md bg-gray-50 text-gray-600">
<p className="text-sm">Grid editor not yet implemented</p>
</div>
);
default:
// Default to text input
return renderField(
<input
id={fieldName}
type="text"
placeholder={field.placeholder}
disabled={disabled}
className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed"
{...register(fieldName, {
required: field.required ? `${field.label || fieldName} is required` : false,
})}
/>
);
}
};
Comment on lines
+355
to
+383
| case 'location': | ||
| return renderField( | ||
| <div className="space-y-2"> | ||
| <input | ||
| id={`${fieldName}-lat`} | ||
| type="number" | ||
| placeholder="Latitude" | ||
| disabled={disabled} | ||
| step="any" | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(`${fieldName}.lat`, { | ||
| required: field.required ? 'Latitude is required' : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| <input | ||
| id={`${fieldName}-lng`} | ||
| type="number" | ||
| placeholder="Longitude" | ||
| disabled={disabled} | ||
| step="any" | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(`${fieldName}.lng`, { | ||
| required: field.required ? 'Longitude is required' : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The location field registers two separate form fields (field.lat and field.lng) using nested paths, but this creates a structure where the value is stored as an object { lat: number, lng: number }. This may not match the expected data structure for location fields. Consider documenting this behavior clearly or ensuring it aligns with how location data is typically represented in the ObjectStack ecosystem.
Comment on lines
+325
to
+353
| case 'file': | ||
| return renderField( | ||
| <input | ||
| id={fieldName} | ||
| type="file" | ||
| disabled={disabled} | ||
| multiple={extendedField.multiple} | ||
| accept={extendedField.accept?.join(',')} | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed file:mr-4 file:py-2 file:px-4 file:rounded-md file:border-0 file:text-sm file:font-semibold file:bg-blue-50 file:text-blue-700 hover:file:bg-blue-100" | ||
| {...register(fieldName, { | ||
| required: field.required ? `${field.label || fieldName} is required` : false, | ||
| })} | ||
| /> | ||
| ); | ||
|
|
||
| case 'image': | ||
| return renderField( | ||
| <input | ||
| id={fieldName} | ||
| type="file" | ||
| disabled={disabled} | ||
| multiple={extendedField.multiple} | ||
| accept={extendedField.accept?.join(',') || 'image/*'} | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed file:mr-4 file:py-2 file:px-4 file:rounded-md file:border-0 file:text-sm file:font-semibold file:bg-blue-50 file:text-blue-700 hover:file:bg-blue-100" | ||
| {...register(fieldName, { | ||
| required: field.required ? `${field.label || fieldName} is required` : false, | ||
| })} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
The file and image fields do not validate file sizes (max_size) or number of files (max_files) as defined in FileFieldMetadata and ImageFieldMetadata. This could allow users to upload excessively large files or more files than intended. Consider adding client-side validation using the maxSize and maxFiles properties from the extended field configuration.
Comment on lines
+13
to
+26
| /** | ||
| * Extended form field with additional properties for complex widgets | ||
| * These properties are not part of the standard FormField schema but may be | ||
| * provided by specific implementations | ||
| */ | ||
| export interface ExtendedFormField extends FormField { | ||
| multiple?: boolean; | ||
| accept?: string[]; | ||
| options?: Array<{ | ||
| label: string; | ||
| value: string; | ||
| disabled?: boolean; | ||
| }>; | ||
| } |
There was a problem hiding this comment.
The ExtendedFormField interface duplicates properties that already exist in the underlying field metadata types from @object-ui/types. For example, FileFieldMetadata, ImageFieldMetadata, LookupFieldMetadata, and SelectFieldMetadata already define multiple, accept, and options properties. This creates ambiguity about which definition takes precedence and could lead to inconsistencies. Consider either: 1) importing and reusing the existing metadata types from @object-ui/types, or 2) clearly documenting that ExtendedFormField is a bridge interface and explaining the relationship.
Comment on lines
+290
to
+598
| describe('Extended field types', () => { | ||
| it('should render currency field with dollar sign', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Pricing', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'price', | ||
| label: 'Price', | ||
| widget: 'currency', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const { container } = render(<FormRenderer schema={schema} />); | ||
| expect(screen.getByLabelText('Price')).toBeInTheDocument(); | ||
| expect(container.querySelector('.absolute')?.textContent).toBe('$'); | ||
| }); | ||
|
|
||
| it('should render percent field with percent sign', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Metrics', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'discount', | ||
| label: 'Discount', | ||
| widget: 'percent', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const { container } = render(<FormRenderer schema={schema} />); | ||
| expect(screen.getByLabelText('Discount')).toBeInTheDocument(); | ||
| expect(container.querySelector('.absolute')?.textContent).toBe('%'); | ||
| }); | ||
|
|
||
| it('should render phone field', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Contact', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'phoneNumber', | ||
| label: 'Phone Number', | ||
| widget: 'phone', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const input = screen.getByLabelText('Phone Number') as HTMLInputElement; | ||
| expect(input.type).toBe('tel'); | ||
| }); | ||
|
|
||
| it('should render markdown field as textarea', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Content', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'content', | ||
| label: 'Content', | ||
| widget: 'markdown', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const textarea = screen.getByLabelText('Content') as HTMLTextAreaElement; | ||
| expect(textarea.tagName).toBe('TEXTAREA'); | ||
| }); | ||
|
|
||
| it('should render html field as textarea', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Content', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'htmlContent', | ||
| label: 'HTML Content', | ||
| widget: 'html', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const textarea = screen.getByLabelText('HTML Content') as HTMLTextAreaElement; | ||
| expect(textarea.tagName).toBe('TEXTAREA'); | ||
| }); | ||
|
|
||
| it('should render file field', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Upload', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'document', | ||
| label: 'Document', | ||
| widget: 'file', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const input = screen.getByLabelText('Document') as HTMLInputElement; | ||
| expect(input.type).toBe('file'); | ||
| }); | ||
|
|
||
| it('should render image field', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Upload', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'avatar', | ||
| label: 'Avatar', | ||
| widget: 'image', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const input = screen.getByLabelText('Avatar') as HTMLInputElement; | ||
| expect(input.type).toBe('file'); | ||
| }); | ||
|
|
||
| it('should render location field with lat/lng inputs', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Location', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'coordinates', | ||
| label: 'Coordinates', | ||
| widget: 'location', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const { container } = render(<FormRenderer schema={schema} />); | ||
| const inputs = container.querySelectorAll('input[type="number"]'); | ||
| expect(inputs.length).toBe(2); | ||
| }); | ||
|
|
||
| it('should render lookup field as select', () => { | ||
| const schema: ExtendedFormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Relations', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: '1', | ||
| fields: [ | ||
| { | ||
| field: 'account', | ||
| label: 'Account', | ||
| widget: 'lookup', | ||
| options: [ | ||
| { label: 'Account 1', value: '1' }, | ||
| { label: 'Account 2', value: '2' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema as FormView} />); | ||
| const select = screen.getByLabelText('Account') as HTMLSelectElement; | ||
| expect(select.tagName).toBe('SELECT'); | ||
| }); | ||
|
|
||
| it('should render user field as select', () => { | ||
| const schema: ExtendedFormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Assignment', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: '1', | ||
| fields: [ | ||
| { | ||
| field: 'assignedTo', | ||
| label: 'Assigned To', | ||
| widget: 'user', | ||
| options: [ | ||
| { label: 'User 1', value: 'user1' }, | ||
| { label: 'User 2', value: 'user2' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema as FormView} />); | ||
| const select = screen.getByLabelText('Assigned To') as HTMLSelectElement; | ||
| expect(select.tagName).toBe('SELECT'); | ||
| }); | ||
|
|
||
| it('should render formula field as disabled input', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Computed', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'total', | ||
| label: 'Total', | ||
| widget: 'formula', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const input = screen.getByLabelText('Total') as HTMLInputElement; | ||
| expect(input.disabled).toBe(true); | ||
| }); | ||
|
|
||
| it('should render object field as JSON textarea', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Data', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'metadata', | ||
| label: 'Metadata', | ||
| widget: 'object', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const textarea = screen.getByLabelText('Metadata') as HTMLTextAreaElement; | ||
| expect(textarea.tagName).toBe('TEXTAREA'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite is missing coverage for several new field types: master_detail, owner, summary, auto_number, vector, and grid. While these share implementation code with other tested types (e.g., master_detail shares with lookup), each widget type should have at least a basic rendering test to ensure the type mapping works correctly and to prevent regressions if implementations diverge in the future.
Comment on lines
+574
to
+597
| it('should render object field as JSON textarea', () => { | ||
| const schema: FormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Data', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: 1, | ||
| fields: [ | ||
| { | ||
| field: 'metadata', | ||
| label: 'Metadata', | ||
| widget: 'object', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema} />); | ||
| const textarea = screen.getByLabelText('Metadata') as HTMLTextAreaElement; | ||
| expect(textarea.tagName).toBe('TEXTAREA'); | ||
| }); |
There was a problem hiding this comment.
The test for the object field only verifies that a textarea is rendered, but does not test the JSON validation functionality. Consider adding tests that verify: 1) valid JSON is accepted, 2) invalid JSON shows an error message, and 3) empty values are allowed when the field is not required.
Comment on lines
+262
to
+281
| case 'percent': | ||
| return renderField( | ||
| <div className="relative"> | ||
| <input | ||
| id={fieldName} | ||
| type="number" | ||
| placeholder={field.placeholder} | ||
| disabled={disabled} | ||
| step="0.01" | ||
| min="0" | ||
| max="100" | ||
| className="w-full pr-8 pl-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(fieldName, { | ||
| required: field.required ? `${field.label || fieldName} is required` : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| <span className="absolute right-3 top-2 text-gray-500">%</span> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The percent field hardcodes min="0" and max="100", but according to PercentFieldMetadata in field-types.ts, the min and max values should be configurable. Some use cases may require negative percentages or values above 100% (e.g., growth rates). Consider reading min/max from the field configuration instead of hardcoding these values.
Comment on lines
+355
to
+383
| case 'location': | ||
| return renderField( | ||
| <div className="space-y-2"> | ||
| <input | ||
| id={`${fieldName}-lat`} | ||
| type="number" | ||
| placeholder="Latitude" | ||
| disabled={disabled} | ||
| step="any" | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(`${fieldName}.lat`, { | ||
| required: field.required ? 'Latitude is required' : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| <input | ||
| id={`${fieldName}-lng`} | ||
| type="number" | ||
| placeholder="Longitude" | ||
| disabled={disabled} | ||
| step="any" | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(`${fieldName}.lng`, { | ||
| required: field.required ? 'Longitude is required' : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The location field creates two input elements for latitude and longitude, but they lack proper labels. The inputs only have placeholder text, which is insufficient for screen readers. Each input should have an associated label element or aria-label attribute for proper accessibility. Consider wrapping each input with a label or adding aria-label attributes.
Comment on lines
+491
to
+547
| it('should render lookup field as select', () => { | ||
| const schema: ExtendedFormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Relations', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: '1', | ||
| fields: [ | ||
| { | ||
| field: 'account', | ||
| label: 'Account', | ||
| widget: 'lookup', | ||
| options: [ | ||
| { label: 'Account 1', value: '1' }, | ||
| { label: 'Account 2', value: '2' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema as FormView} />); | ||
| const select = screen.getByLabelText('Account') as HTMLSelectElement; | ||
| expect(select.tagName).toBe('SELECT'); | ||
| }); | ||
|
|
||
| it('should render user field as select', () => { | ||
| const schema: ExtendedFormView = { | ||
| type: 'simple', | ||
| sections: [ | ||
| { | ||
| label: 'Assignment', | ||
| collapsible: false, | ||
| collapsed: false, | ||
| columns: '1', | ||
| fields: [ | ||
| { | ||
| field: 'assignedTo', | ||
| label: 'Assigned To', | ||
| widget: 'user', | ||
| options: [ | ||
| { label: 'User 1', value: 'user1' }, | ||
| { label: 'User 2', value: 'user2' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| render(<FormRenderer schema={schema as FormView} />); | ||
| const select = screen.getByLabelText('Assigned To') as HTMLSelectElement; | ||
| expect(select.tagName).toBe('SELECT'); | ||
| }); |
There was a problem hiding this comment.
The tests for select-based fields (lookup, user) do not verify that the multiple property works correctly or that the handleMultipleSelectValue function properly converts selected options to an array. Consider adding tests that verify multi-select behavior, including checking that the correct value is set when multiple options are selected.
Comment on lines
10
to
12
| import { UseFormReturn } from 'react-hook-form'; | ||
| import type { FormField } from '@objectstack/spec/ui'; | ||
|
|
There was a problem hiding this comment.
The PR title mentions "OpenStack" but the correct product name is "ObjectStack" (based on the package names @objectstack/spec and repository context). This appears to be a typo in the PR title/description, though not in the actual code.
Comment on lines
+243
to
+260
| case 'currency': | ||
| return renderField( | ||
| <div className="relative"> | ||
| <span className="absolute left-3 top-2 text-gray-500">$</span> | ||
| <input | ||
| id={fieldName} | ||
| type="number" | ||
| placeholder={field.placeholder} | ||
| disabled={disabled} | ||
| step="0.01" | ||
| className="w-full pl-8 pr-3 py-2 border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-blue-500 disabled:bg-gray-100 disabled:cursor-not-allowed" | ||
| {...register(fieldName, { | ||
| required: field.required ? `${field.label || fieldName} is required` : false, | ||
| valueAsNumber: true, | ||
| })} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The currency field hardcodes the dollar sign ($) symbol, but the field-types.ts definition includes a currency property to specify different currencies (e.g., EUR, GBP). Consider using the currency property from the field configuration to display the appropriate currency symbol dynamically, or at minimum document this limitation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FormRenderer (PR #179) supported 11 of 28 field types defined in
packages/types/src/field-types.ts. This extends FieldFactory to handle the remaining 17 types.Added Widget Types
Basic inputs:
currency($ prefix),percent(% suffix),phone,markdown,html(monospace textareas)File uploads:
file,image(with accept filtering)Complex types:
location(lat/lng dual inputs),object(JSON-validated textarea)Relationships:
lookup,master_detail,user,owner(select with options)Read-only computed:
formula,summary,auto_number,vector(disabled inputs)Placeholder:
grid(not yet implemented)Extended Field Interface
Base
FormFieldfrom@objectstack/speclacks properties needed for complex widgets. AddedExtendedFormField:Usage:
Implementation Notes
HTMLCollectionOf<HTMLOptionElement>→ array viasetValueAsfield.lat,field.lng)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.