Conversation
…to feat/components
…/, fields/, and layout/ folders
…DE-MA/FormKit-UI into feat/components
…40+ countries, flagcdn.com flags)
… with client-side validation
…max/step config options
…MultiSelectField style
…pdowns and pickers
There was a problem hiding this comment.
Pull request overview
Refactors the FormKit UI codebase toward a Component–Hook–Model (CHM) architecture and introduces a framework-free core/ layer, while replacing legacy components/adapters with new field/layout/form implementations and tests.
Changes:
- Added
src/core/utilities (types, validation, conditionals, grid, i18n, errors) with new unit tests and coverage tooling. - Introduced new field/layout/form components (DynamicForm internals, standardized Field* components) and removed legacy components/adapters/errors modules.
- Added/updated changesets and package scripts/deps for coverage reporting.
Reviewed changes
Copilot reviewed 92 out of 128 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/tests/useAsyncValidation.test.ts | Adds tests for async validation behavior (debounce/abort/error handling). |
| src/hooks/FormKitContext.ts | Introduces shared React context + hook under hooks layer. |
| src/errors/types.ts | Removes legacy error type definitions module. |
| src/errors/index.ts | Removes legacy errors barrel export. |
| src/errors/errorFormatters.ts | Removes legacy error formatting utilities. |
| src/core/validator.ts | Adds framework-free validation helpers (Zod mapping, sync/field validation, emptiness check). |
| src/core/types.ts | Adds framework-free core types/constants (FieldType, values, validation mode, debounce consts). |
| src/core/schema-helpers.ts | Adds helpers to create/merge Zod schemas. |
| src/core/index.ts | Adds barrel exports for the core module. |
| src/core/i18n.ts | Adds framework-free translation types + lookup/interpolation utility. |
| src/core/grid.ts | Adds grid class generation utilities (responsive Tailwind helpers). |
| src/core/errors.ts | Adds framework-free custom error classes. |
| src/core/conditional.ts | Adds conditional visibility evaluation utilities. |
| src/core/tests/validator.test.ts | Adds tests for validator utilities. |
| src/core/tests/schema-helpers.test.ts | Adds tests for schema helpers. |
| src/core/tests/grid.test.ts | Adds tests for grid helpers. |
| src/core/tests/conditional.test.ts | Adds tests for conditional logic. |
| src/components/layout/index.ts | Adds layout barrel exports. |
| src/components/layout/tests/FormSection.test.tsx | Adds tests for FormSection layout + grid behavior. |
| src/components/layout/FormSection.tsx | Adds section rendering with fieldset/legend and grid/colSpan support. |
| src/components/layout/FormActions.tsx | Adds standardized action buttons with i18n + loading state. |
| src/components/layout/FieldLabel.tsx | Adds standardized label/legend component with required indicator + i18n. |
| src/components/layout/FieldGroup.tsx | Adds field grouping component using fieldset/legend. |
| src/components/layout/FieldError.tsx | Adds standardized accessible field error rendering. |
| src/components/index.ts | Reworks component exports into CHM modules and re-exports provider/context hook. |
| src/components/form/index.ts | Adds form module barrel exports. |
| src/components/form/FormStepper.tsx | Adds stepper UI for wizard mode with accessibility attributes. |
| src/components/form/DynamicFormStep.tsx | Adds per-step rendering wrapper for wizard mode. |
| src/components/fields/index.ts | Adds fields module barrel exports. |
| src/components/fields/tests/TimeField.test.tsx | Adds tests for TimeField behavior/a11y. |
| src/components/fields/tests/TagsField.test.tsx | Adds tests for TagsField add/remove/limits behavior. |
| src/components/fields/tests/SliderField.test.tsx | Adds tests for SliderField behavior and a11y attrs. |
| src/components/fields/tests/RatingField.test.tsx | Adds tests for RatingField interactions + keyboard support. |
| src/components/fields/tests/RangeSliderField.test.tsx | Adds tests for RangeSliderField behavior. |
| src/components/fields/tests/PhoneField.test.tsx | Adds tests for PhoneField interactions and structure. |
| src/components/fields/tests/PasswordField.test.tsx | Adds tests for PasswordField toggle + a11y. |
| src/components/fields/tests/OTPField.test.tsx | Adds tests for OTPField input/paste/navigation. |
| src/components/fields/tests/MultiSelectField.test.tsx | Adds tests for MultiSelectField interactions and a11y. |
| src/components/fields/tests/DateTimeField.test.tsx | Adds tests for DateTimeField behavior/a11y. |
| src/components/fields/TextareaField.tsx | Adds new textarea field implementation using FormKit context. |
| src/components/fields/TextField.tsx | Adds new text/email/number input field implementation. |
| src/components/fields/TagsField.tsx | Adds tags input with limits/duplicates/paste support. |
| src/components/fields/SwitchField.tsx | Adds switch field implementation with role="switch". |
| src/components/fields/SliderField.tsx | Adds range slider field with optional numeric input. |
| src/components/fields/RatingField.tsx | Adds rating field with stars, half-step, and keyboard controls. |
| src/components/fields/RadioGroupField.tsx | Adds radio group field using fieldset/legend + radiogroup. |
| src/components/fields/PasswordField.tsx | Adds password field with show/hide toggle and improved styling. |
| src/components/fields/OTPField.tsx | Adds OTP input field with focus management and paste support. |
| src/components/fields/FileField.tsx | Adds file upload field with type/size validation and i18n hints. |
| src/components/fields/Field.tsx | Adds field router (type-based rendering + conditional visibility). |
| src/components/fields/CheckboxField.tsx | Adds checkbox field with custom visuals + hidden native input. |
| src/components/context/index.ts | Adds context module barrel exports. |
| src/components/context/I18nContext.tsx | Adds i18n provider/context with deep-merge support. |
| src/components/context/FormKitContext.tsx | Adds provider component bridging to hooks/FormKitContext. |
| src/components/tests/ErrorMessage.test.tsx | Removes tests for legacy ErrorMessage component. |
| src/components/Textarea.tsx | Removes legacy Textarea component. |
| src/components/Select.tsx | Removes legacy Select component. |
| src/components/RadioGroup.tsx | Removes legacy RadioGroup component. |
| src/components/Input.tsx | Removes legacy Input component. |
| src/components/FormField.tsx | Removes legacy FormField wrapper component. |
| src/components/ErrorMessage.tsx | Removes legacy ErrorMessage component. |
| src/components/Checkbox.tsx | Removes legacy Checkbox component. |
| src/adapters/zod.ts | Removes legacy Zod adapter. |
| src/adapters/react-hook-form.ts | Removes legacy React Hook Form adapter. |
| src/adapters/index.ts | Removes legacy adapters barrel export. |
| src/adapters/tests/zod.test.ts | Removes tests for legacy Zod adapter. |
| package.json | Adds Vitest coverage dependency and test:cov script. |
| .changeset/instructions-refactor.md | Adds changeset describing refactor + testing/coverage claims. |
| .changeset/improved-field-styles.md | Adds changeset describing style improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Column span class | ||
| const colSpanClass = config.colSpan ? `col-span-${config.colSpan}` : ''; | ||
| const wrapperClass = `formkit-field ${colSpanClass} ${config.className ?? ''}`.trim(); | ||
|
|
||
| // Route to appropriate field component based on type | ||
| const renderField = (): JSX.Element => { |
There was a problem hiding this comment.
config.colSpan can be a responsive config object (e.g. { default: 12, md: 6 }), but this code interpolates it into a class string, producing col-span-[object Object]. Also, FormSection already wraps each rendered Field in a div with a col-span class, so this can lead to duplicated/conflicting col-span classes. A concrete fix is to remove col-span handling from Field entirely (let layout components own grid span), or switch to getColSpanClass(config.colSpan) and ensure span is applied in only one place.
| // Column span class | |
| const colSpanClass = config.colSpan ? `col-span-${config.colSpan}` : ''; | |
| const wrapperClass = `formkit-field ${colSpanClass} ${config.className ?? ''}`.trim(); | |
| // Route to appropriate field component based on type | |
| const renderField = (): JSX.Element => { | |
| // Base wrapper class (layout components like FormSection own grid span) | |
| const wrapperClass = `formkit-field ${config.className ?? ''}`.trim(); | |
| // Route to appropriate field component based on type | |
| const renderField = (): JSX.Element => { | |
| const renderField = (): JSX.Element => { |
| [showError ? errorId : null, config.description ? descId : null].filter(Boolean).join(' ') || | ||
| undefined; |
There was a problem hiding this comment.
aria-describedby always includes descId when config.description exists, but the description element is not rendered when showError is true. This causes aria-describedby to reference a non-existent element, which is an accessibility defect. Fix by only including descId when the description is actually rendered (e.g. (!showError && config.description ? descId : null)), or always render the description node (optionally visually hidden) so the reference remains valid. The same pattern appears in other field components in this PR (e.g., OTP/Tags/Slider/Rating) and should be made consistent.
| [showError ? errorId : null, config.description ? descId : null].filter(Boolean).join(' ') || | |
| undefined; | |
| [showError ? errorId : null, !showError && config.description ? descId : null] | |
| .filter(Boolean) | |
| .join(' ') || undefined; |
| {config.description && !showError && ( | ||
| <p id={descId} className="text-xs text-gray-500 mt-1"> | ||
| {config.description} | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
aria-describedby always includes descId when config.description exists, but the description element is not rendered when showError is true. This causes aria-describedby to reference a non-existent element, which is an accessibility defect. Fix by only including descId when the description is actually rendered (e.g. (!showError && config.description ? descId : null)), or always render the description node (optionally visually hidden) so the reference remains valid. The same pattern appears in other field components in this PR (e.g., OTP/Tags/Slider/Rating) and should be made consistent.
| /** | ||
| * Field errors mapping (field key → error message) | ||
| */ | ||
| export type FieldErrors<T extends FormValues = FormValues> = Partial<Record<keyof T, string>>; |
There was a problem hiding this comment.
mapZodErrors's generic T is not inferable from the current parameter type (z.ZodError without a type argument), and the implementation produces string paths like 'user.email' / 'items.0' that cannot be represented safely as keyof T. This can cause TypeScript callers/tests to either fail type-checking or rely on unsafe casts. Consider changing the signature to accept z.ZodError<T> (and default T), and loosening FieldErrors to support string paths (e.g. Partial<Record<string, string>>) or splitting root-level vs nested paths explicitly.
| * } | ||
| * ``` | ||
| */ | ||
| export function mapZodErrors<T extends FormValues>(zodError: z.ZodError): FieldErrors<T> { |
There was a problem hiding this comment.
mapZodErrors's generic T is not inferable from the current parameter type (z.ZodError without a type argument), and the implementation produces string paths like 'user.email' / 'items.0' that cannot be represented safely as keyof T. This can cause TypeScript callers/tests to either fail type-checking or rely on unsafe casts. Consider changing the signature to accept z.ZodError<T> (and default T), and loosening FieldErrors to support string paths (e.g. Partial<Record<string, string>>) or splitting root-level vs nested paths explicitly.
|
|
||
| for (const issue of zodError.issues) { | ||
| // Get the field path (first element for simple fields, joined for nested) | ||
| const fieldKey = issue.path.length > 0 ? issue.path.join('.') : '_root'; |
There was a problem hiding this comment.
mapZodErrors's generic T is not inferable from the current parameter type (z.ZodError without a type argument), and the implementation produces string paths like 'user.email' / 'items.0' that cannot be represented safely as keyof T. This can cause TypeScript callers/tests to either fail type-checking or rely on unsafe casts. Consider changing the signature to accept z.ZodError<T> (and default T), and loosening FieldErrors to support string paths (e.g. Partial<Record<string, string>>) or splitting root-level vs nested paths explicitly.
|
|
||
| // Only keep the first error per field | ||
| if (!(fieldKey in errors)) { | ||
| errors[fieldKey as keyof T] = issue.message; |
There was a problem hiding this comment.
mapZodErrors's generic T is not inferable from the current parameter type (z.ZodError without a type argument), and the implementation produces string paths like 'user.email' / 'items.0' that cannot be represented safely as keyof T. This can cause TypeScript callers/tests to either fail type-checking or rely on unsafe casts. Consider changing the signature to accept z.ZodError<T> (and default T), and loosening FieldErrors to support string paths (e.g. Partial<Record<string, string>>) or splitting root-level vs nested paths explicitly.
| @@ -0,0 +1,14 @@ | |||
| --- | |||
There was a problem hiding this comment.
The first line contains a BOM/zero-width character before --- (---). Changesets frontmatter parsing can break when the file doesn't start with a literal ---. Remove the BOM so line 1 begins exactly with ---.
| --- | |
| --- |
| 'useFormKitContext must be used within a DynamicForm. ' + | ||
| 'Make sure your component is rendered inside <DynamicForm>.', |
There was a problem hiding this comment.
The thrown error message refers specifically to DynamicForm, but the JSDoc above says it throws if used outside FormKitProvider. This is confusing for consumers who may use the provider directly (or if the provider component name changes). Align the message with the actual contract (e.g., reference FormKitProvider and optionally mention DynamicForm as a common wrapper).
| 'useFormKitContext must be used within a DynamicForm. ' + | |
| 'Make sure your component is rendered inside <DynamicForm>.', | |
| 'useFormKitContext must be used within a FormKitProvider (e.g., via DynamicForm). ' + | |
| 'Make sure your component is rendered inside a FormKitProvider or <DynamicForm>.', |
src/components/form/FormStepper.tsx
Outdated
|
|
||
| return ( | ||
| <li | ||
| key={step.title} |
There was a problem hiding this comment.
Using step.title as the React key can break list reconciliation if titles are not unique or change (e.g., localized titles). Prefer a stable unique identifier from StepConfig (like step.id if available), or fall back to index only if the order is stable and no better key exists.
| key={step.title} | |
| key={index} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 128 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| expect(() => { | ||
| renderHook(() => useFormKitContext()); | ||
| }).toThrow('useFormKitContext must be used within a DynamicForm'); |
There was a problem hiding this comment.
The expected error message no longer matches the actual message thrown by useFormKitContext (the hook now includes additional guidance text). Update the assertion to match the full message, or assert a stable substring/regex (e.g., toThrow(/useFormKitContext must be used within a DynamicForm/)) to avoid brittle failures.
| }).toThrow('useFormKitContext must be used within a DynamicForm'); | |
| }).toThrow(/useFormKitContext must be used within a DynamicForm/); |
| export function mapZodErrors<T extends FormValues>(zodError: z.ZodError): FieldErrors<T> { | ||
| const errors: FieldErrors<T> = {}; | ||
|
|
||
| for (const issue of zodError.issues) { | ||
| // Get the field path (first element for simple fields, joined for nested) | ||
| const fieldKey = issue.path.length > 0 ? issue.path.join('.') : '_root'; | ||
|
|
||
| // Only keep the first error per field | ||
| if (!(fieldKey in errors)) { | ||
| errors[fieldKey as keyof T] = issue.message; | ||
| } | ||
| } |
There was a problem hiding this comment.
mapZodErrors can emit keys like 'user.email', 'items.0', and '_root', but FieldErrors<T> is typed as Partial<Record<keyof T, string>>, which cannot represent these string paths safely. This can mislead API consumers and forces unsafe casting. Consider changing FieldErrors to a Record<string, string> (or a dedicated Path<T> type if you want strongly-typed nested paths) and explicitly include '_root' in the type if supported.
| export function mergeSchemas<T extends z.ZodRawShape[]>(schemas: { | ||
| [K in keyof T]: z.ZodObject<T[K]>; | ||
| }): z.ZodObject<T[number]> { | ||
| if (schemas.length === 0) { | ||
| return z.object({}); | ||
| } | ||
|
|
||
| let merged = schemas[0]; | ||
| for (let i = 1; i < schemas.length; i++) { | ||
| merged = merged.merge(schemas[i]) as typeof merged; | ||
| } | ||
|
|
||
| return merged as z.ZodObject<T[number]>; |
There was a problem hiding this comment.
The generic typing here is unsound in a few ways: (1) T[number] represents a union of shapes, but merge() produces an intersection-like merged shape; (2) for an empty array, returning z.object({}) doesn’t match the declared return type; (3) the parameter type is more complex than needed for callers. A more accurate and ergonomic API is mergeSchemas(schemas: z.AnyZodObject[]): z.AnyZodObject (optionally with overloads), which also cleanly supports mergeSchemas([]) without type-casting.
| export function mergeSchemas<T extends z.ZodRawShape[]>(schemas: { | |
| [K in keyof T]: z.ZodObject<T[K]>; | |
| }): z.ZodObject<T[number]> { | |
| if (schemas.length === 0) { | |
| return z.object({}); | |
| } | |
| let merged = schemas[0]; | |
| for (let i = 1; i < schemas.length; i++) { | |
| merged = merged.merge(schemas[i]) as typeof merged; | |
| } | |
| return merged as z.ZodObject<T[number]>; | |
| export function mergeSchemas(schemas: z.AnyZodObject[]): z.AnyZodObject { | |
| if (schemas.length === 0) { | |
| return z.object({}); | |
| } | |
| let merged: z.AnyZodObject = schemas[0]; | |
| for (let i = 1; i < schemas.length; i++) { | |
| merged = merged.merge(schemas[i]); | |
| } | |
| return merged; |
| <svg className="w-6 h-6 sm:w-8 sm:h-8" viewBox="0 0 24 24" fill="none"> | ||
| <defs> | ||
| <linearGradient id="halfFill"> | ||
| <stop offset="50%" stopColor="currentColor" /> | ||
| <stop offset="50%" stopColor="transparent" /> | ||
| </linearGradient> | ||
| </defs> | ||
| <path | ||
| d="M12 2l3.09 6.26L22 9.27l-5 4.87 1.18 6.88L12 17.77l-6.18 3.25L7 14.14 2 9.27l6.91-1.01L12 2z" | ||
| fill="url(#halfFill)" |
There was a problem hiding this comment.
The gradient id="halfFill" is not unique. Rendering multiple RatingField instances on the same page can cause SVG ID collisions, leading to incorrect fills (the url(#halfFill) reference may bind to a different instance). Use a unique ID per component instance (e.g., via useId() or by parameterizing an ID) and reference that in the fill="url(#...)".
| <div | ||
| onClick={handleContainerClick} | ||
| className={` | ||
| formkit-tags-container | ||
| flex flex-wrap gap-2 p-2 | ||
| border rounded-lg | ||
| min-h-[42px] | ||
| cursor-text | ||
| transition-all duration-150 | ||
| ${ | ||
| showError | ||
| ? 'border-red-500 focus-within:ring-2 focus-within:ring-red-500' | ||
| : 'border-gray-300 hover:border-gray-400 focus-within:border-blue-500 focus-within:ring-2 focus-within:ring-blue-500' | ||
| } | ||
| ${isDisabled ? 'bg-gray-100 cursor-not-allowed' : 'bg-white'} | ||
| `} | ||
| aria-describedby={describedBy} | ||
| > |
There was a problem hiding this comment.
aria-describedby is applied to the container <div>, but the actual focusable control is the <input> inside. Screen readers typically announce descriptions for the focused element, so error/description content may not be read when users tab into the input. Move/add aria-describedby={describedBy} to the input (and keep it in sync with error/description rendering).
| // Context (internal use only) | ||
| export { default as FormKitProvider, useFormKitContext } from './context/FormKitContext'; |
There was a problem hiding this comment.
This exports FormKitProvider/useFormKitContext from the components entrypoint while the context file comments state it’s “Internal only — not exported from public API”. If the intent is truly internal, these should not be re-exported from the public components barrel; otherwise, update the internal-only documentation to reflect that these are supported exports.
| // Context (internal use only) | |
| export { default as FormKitProvider, useFormKitContext } from './context/FormKitContext'; | |
| // Context (internal use only - not exported from public API) |
| // Column span class | ||
| const colSpanClass = config.colSpan ? `col-span-${config.colSpan}` : ''; | ||
| const wrapperClass = `formkit-field ${colSpanClass} ${config.className ?? ''}`.trim(); |
There was a problem hiding this comment.
FormSection already applies colSpan via getColSpanClass(field.colSpan) and wraps each field, so this duplicates layout responsibility and can also generate invalid classes if config.colSpan is not a simple number (e.g., responsive object). Consider removing colSpan handling from Field entirely (preferred, since layout is handled by FormSection), or use the same getColSpanClass helper consistently and avoid double-wrapping.
| // Column span class | |
| const colSpanClass = config.colSpan ? `col-span-${config.colSpan}` : ''; | |
| const wrapperClass = `formkit-field ${colSpanClass} ${config.className ?? ''}`.trim(); | |
| const wrapperClass = `formkit-field ${config.className ?? ''}`.trim(); |
|
No description provided.