From 863668525281f50ed7ae232f3b8c19ef040f69cf Mon Sep 17 00:00:00 2001 From: Jarrod Flesch <30633324+JarrodMFlesch@users.noreply.github.com> Date: Thu, 28 Mar 2024 13:15:44 -0400 Subject: [PATCH] fix: simplifies field error paths (#5504) --- .github/workflows/main.yml | 2 +- packages/payload/src/admin/forms/Form.ts | 1 - packages/payload/src/admin/forms/Label.ts | 1 + packages/payload/src/fields/validations.ts | 2 +- packages/ui/src/fields/Array/ArrayRow.tsx | 7 +- packages/ui/src/fields/Array/index.tsx | 68 ++++++++++--------- packages/ui/src/fields/Blocks/BlockRow.tsx | 5 +- packages/ui/src/fields/Blocks/index.tsx | 9 ++- packages/ui/src/fields/Collapsible/index.tsx | 2 +- packages/ui/src/fields/Group/index.tsx | 10 +-- packages/ui/src/forms/FieldLabel/index.tsx | 5 +- .../ui/src/forms/Form/mergeServerFormState.ts | 24 ------- .../addFieldStatePromise.ts | 38 +++++++---- .../src/forms/buildStateFromSchema/index.tsx | 2 +- .../buildStateFromSchema/iterateFields.ts | 9 +-- packages/ui/src/forms/useField/index.tsx | 2 + packages/ui/src/forms/useField/types.ts | 1 + 17 files changed, 97 insertions(+), 91 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7d5b00b3ef..a250aade23 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -220,7 +220,7 @@ jobs: - access-control # - admin - auth - # - field-error-states + - field-error-states # - fields-relationship # - fields - fields/lexical diff --git a/packages/payload/src/admin/forms/Form.ts b/packages/payload/src/admin/forms/Form.ts index 8501b5ff29..a540cb9540 100644 --- a/packages/payload/src/admin/forms/Form.ts +++ b/packages/payload/src/admin/forms/Form.ts @@ -8,7 +8,6 @@ export type Data = { export type Row = { blockType?: string collapsed?: boolean - errorPaths?: string[] id: string } diff --git a/packages/payload/src/admin/forms/Label.ts b/packages/payload/src/admin/forms/Label.ts index 592bc94c8a..1c5e39bae0 100644 --- a/packages/payload/src/admin/forms/Label.ts +++ b/packages/payload/src/admin/forms/Label.ts @@ -1,5 +1,6 @@ export type LabelProps = { CustomLabel?: React.ReactNode + as?: 'label' | 'span' htmlFor?: string label?: Record | false | string required?: boolean diff --git a/packages/payload/src/fields/validations.ts b/packages/payload/src/fields/validations.ts index 3477537803..2e57e824e7 100644 --- a/packages/payload/src/fields/validations.ts +++ b/packages/payload/src/fields/validations.ts @@ -229,7 +229,7 @@ const validateArrayLength = ( ) => { const { maxRows, minRows, required, t } = options - const arrayLength = Array.isArray(value) ? value.length : 0 + const arrayLength = Array.isArray(value) ? value.length : value || 0 if (!required && arrayLength === 0) return true diff --git a/packages/ui/src/fields/Array/ArrayRow.tsx b/packages/ui/src/fields/Array/ArrayRow.tsx index f592a24a6a..faf6a4453f 100644 --- a/packages/ui/src/fields/Array/ArrayRow.tsx +++ b/packages/ui/src/fields/Array/ArrayRow.tsx @@ -22,6 +22,7 @@ type ArrayRowProps = UseDraggableSortableReturn & { CustomRowLabel?: React.ReactNode addRow: (rowIndex: number) => void duplicateRow: (rowIndex: number) => void + errorCount: number fieldMap: FieldMap forceRender?: boolean hasMaxRows?: boolean @@ -35,6 +36,7 @@ type ArrayRowProps = UseDraggableSortableReturn & { row: Row rowCount: number rowIndex: number + schemaPath: string setCollapse: (rowID: string, collapsed: boolean) => void } @@ -43,6 +45,7 @@ export const ArrayRow: React.FC = ({ addRow, attributes, duplicateRow, + errorCount, fieldMap, forceRender = false, hasMaxRows, @@ -57,6 +60,7 @@ export const ArrayRow: React.FC = ({ row, rowCount, rowIndex, + schemaPath, setCollapse, setNodeRef, transform, @@ -70,7 +74,6 @@ export const ArrayRow: React.FC = ({ '0', )}` - const errorCount = row.errorPaths?.length const fieldHasErrors = errorCount > 0 && hasSubmitted const classNames = [ @@ -134,7 +137,7 @@ export const ArrayRow: React.FC = ({ path={path} permissions={permissions?.fields} readOnly={readOnly} - schemaPath={parentPath} + schemaPath={schemaPath} /> diff --git a/packages/ui/src/fields/Array/index.tsx b/packages/ui/src/fields/Array/index.tsx index 6fdf028599..e98e071208 100644 --- a/packages/ui/src/fields/Array/index.tsx +++ b/packages/ui/src/fields/Array/index.tsx @@ -112,6 +112,7 @@ export const _ArrayField: React.FC = (props) => { const { path: pathFromContext } = useFieldProps() const { + errorPaths, path, rows = [], schemaPath, @@ -180,10 +181,8 @@ export const _ArrayField: React.FC = (props) => { const hasMaxRows = maxRows && rows.length >= maxRows - const fieldErrorCount = - rows.reduce((total, row) => total + (row?.errorPaths?.length || 0), 0) + (valid ? 0 : 1) - - const fieldHasErrors = submitted && fieldErrorCount > 0 + const fieldErrorCount = errorPaths.length + const fieldHasErrors = submitted && errorPaths.length > 0 const showRequired = readOnly && rows.length === 0 const showMinRows = rows.length < minRows || (required && rows.length === 0) @@ -209,7 +208,7 @@ export const _ArrayField: React.FC = (props) => {

- +

{fieldHasErrors && fieldErrorCount > 0 && ( @@ -247,32 +246,39 @@ export const _ArrayField: React.FC = (props) => { ids={rows.map((row) => row.id)} onDragEnd={({ moveFromIndex, moveToIndex }) => moveRow(moveFromIndex, moveToIndex)} > - {rows.map((row, i) => ( - - {(draggableSortableItemProps) => ( - - )} - - ))} + {rows.map((row, i) => { + const rowErrorCount = errorPaths?.filter((errorPath) => + errorPath.startsWith(`${path}.${i}.`), + ).length + return ( + + {(draggableSortableItemProps) => ( + + )} + + ) + })} {!valid && ( {showRequired && ( diff --git a/packages/ui/src/fields/Blocks/BlockRow.tsx b/packages/ui/src/fields/Blocks/BlockRow.tsx index 5b6518816c..5830a96323 100644 --- a/packages/ui/src/fields/Blocks/BlockRow.tsx +++ b/packages/ui/src/fields/Blocks/BlockRow.tsx @@ -24,6 +24,7 @@ type BlockFieldProps = UseDraggableSortableReturn & { block: ReducedBlock blocks: ReducedBlock[] duplicateRow: (rowIndex: number) => void + errorCount: number forceRender?: boolean hasMaxRows?: boolean indexPath: string @@ -46,6 +47,7 @@ export const BlockRow: React.FC = ({ block, blocks, duplicateRow, + errorCount, forceRender, hasMaxRows, labels, @@ -67,8 +69,7 @@ export const BlockRow: React.FC = ({ const { i18n } = useTranslation() const hasSubmitted = useFormSubmitted() - const errorCount = row.errorPaths?.length - const fieldHasErrors = errorCount > 0 && hasSubmitted + const fieldHasErrors = hasSubmitted && errorCount > 0 const classNames = [ `${baseClass}__row`, diff --git a/packages/ui/src/fields/Blocks/index.tsx b/packages/ui/src/fields/Blocks/index.tsx index 57828b27d6..d674e86cab 100644 --- a/packages/ui/src/fields/Blocks/index.tsx +++ b/packages/ui/src/fields/Blocks/index.tsx @@ -113,6 +113,7 @@ const _BlocksField: React.FC = (props) => { const { path: pathFromContext } = useFieldProps() const { + errorPaths, path, permissions, rows = [], @@ -192,7 +193,7 @@ const _BlocksField: React.FC = (props) => { const hasMaxRows = maxRows && rows.length >= maxRows - const fieldErrorCount = rows.reduce((total, row) => total + (row?.errorPaths?.length || 0), 0) + const fieldErrorCount = errorPaths.length const fieldHasErrors = submitted && fieldErrorCount + (valid ? 0 : 1) > 0 const showMinRows = rows.length < minRows || (required && rows.length === 0) @@ -219,7 +220,7 @@ const _BlocksField: React.FC = (props) => {

- +

{fieldHasErrors && fieldErrorCount > 0 && ( @@ -262,6 +263,9 @@ const _BlocksField: React.FC = (props) => { const blockToRender = blocks.find((block) => block.slug === blockType) if (blockToRender) { + const rowErrorCount = errorPaths.filter((errorPath) => + errorPath.startsWith(`${path}.${i}`), + ).length return ( {(draggableSortableItemProps) => ( @@ -271,6 +275,7 @@ const _BlocksField: React.FC = (props) => { block={blockToRender} blocks={blocks} duplicateRow={duplicateRow} + errorCount={rowErrorCount} forceRender={forceRender} hasMaxRows={hasMaxRows} indexPath={indexPath} diff --git a/packages/ui/src/fields/Collapsible/index.tsx b/packages/ui/src/fields/Collapsible/index.tsx index f6ba06f8c5..02993b8c97 100644 --- a/packages/ui/src/fields/Collapsible/index.tsx +++ b/packages/ui/src/fields/Collapsible/index.tsx @@ -1,5 +1,5 @@ 'use client' -import type { DocumentPreferences, FieldBase, RowLabel as RowLabelType } from 'payload/types' +import type { DocumentPreferences, FieldBase } from 'payload/types' import React, { Fragment, useCallback, useEffect, useState } from 'react' diff --git a/packages/ui/src/fields/Group/index.tsx b/packages/ui/src/fields/Group/index.tsx index e3a96cd1ef..ad4356eca8 100644 --- a/packages/ui/src/fields/Group/index.tsx +++ b/packages/ui/src/fields/Group/index.tsx @@ -12,8 +12,9 @@ import type { FormFieldBase } from '../shared/index.js' import { useCollapsible } from '../../elements/Collapsible/provider.js' import { ErrorPill } from '../../elements/ErrorPill/index.js' import { useFieldProps } from '../../forms/FieldPropsProvider/index.js' +import { useFormSubmitted } from '../../forms/Form/context.js' import { RenderFields } from '../../forms/RenderFields/index.js' -import { WatchChildErrors } from '../../forms/WatchChildErrors/index.js' +import { useField } from '../../forms/useField/index.js' import { withCondition } from '../../forms/withCondition/index.js' import { useTranslation } from '../../providers/Translation/index.js' import { useRow } from '../Row/provider.js' @@ -53,14 +54,15 @@ const GroupField: React.FC = (props) => { const isWithinGroup = useGroup() const isWithinRow = useRow() const isWithinTab = useTabs() - const [errorCount, setErrorCount] = React.useState(undefined) - const fieldHasErrors = errorCount > 0 + const { errorPaths } = useField({ path }) + const submitted = useFormSubmitted() + const errorCount = errorPaths.length + const fieldHasErrors = submitted && errorCount > 0 const isTopLevel = !(isWithinCollapsible || isWithinGroup || isWithinRow) return ( -
= (props) => { const { + as: Element = 'label', htmlFor: htmlForFromProps, label: labelFromProps, required = false, @@ -27,10 +28,10 @@ const DefaultFieldLabel: React.FC = (props) => { if (labelFromProps) { return ( - + ) } diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index 604e829f1c..92d4111cbd 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -52,30 +52,6 @@ export const mergeServerFormState = ( } }) - /** - * Handle rows - */ - if (Array.isArray(newFieldState.rows) && newFieldState?.rows.length) { - let i = 0 - for (const row of newFieldState.rows) { - const incomingRow = incomingState[path]?.rows?.[i] - if (incomingRow) { - const errorPathsRowResult = mergeErrorPaths( - row.errorPaths, - incomingRow.errorPaths as unknown as string[], - ) - if (errorPathsRowResult.result) { - if (errorPathsResult.changed) { - changed = true - } - incomingRow.errorPaths = errorPathsRowResult.result - } - } - - i++ - } - } - // Conditions don't work if we don't memcopy the new state, as the object references would otherwise be the same newState[path] = { ...newFieldState } }) diff --git a/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts b/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts index 67533c4f6d..a3df134147 100644 --- a/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts +++ b/packages/ui/src/forms/buildStateFromSchema/addFieldStatePromise.ts @@ -18,13 +18,14 @@ const ObjectId = (ObjectIdImport.default || ObjectIdImport) as unknown as typeof ObjectIdImport.default export type AddFieldStatePromiseArgs = { + addErrorPathToParent: (path: string) => void /** * if all parents are localized, then the field is localized */ anyParentLocalized?: boolean data: Data - errorPaths: string[] field: NonPresentationalField + fieldIndex: number /** * You can use this to filter down to only `localized` fields that require translation (type: text, textarea, etc.). Another plugin might want to look for only `point` type fields to do some GIS function. With the filter function you can go in like a surgeon. */ @@ -72,9 +73,9 @@ export type AddFieldStatePromiseArgs = { export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Promise => { const { id, + addErrorPathToParent: addErrorPathToParentArg, anyParentLocalized = false, data, - errorPaths: parentErrorPaths = [], field, filter, forceFullValue = false, @@ -92,7 +93,7 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom } = args if (fieldAffectsData(field)) { - const validate = operation === 'update' ? field.validate : undefined + const validate = field.validate const fieldState: FormField = { errorPaths: [], @@ -139,14 +140,21 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom }) } + const addErrorPathToParent = (errorPath: string) => { + if (typeof addErrorPathToParentArg === 'function') { + addErrorPathToParentArg(errorPath) + } + + if (!fieldState.errorPaths.includes(errorPath)) { + fieldState.errorPaths.push(errorPath) + fieldState.valid = false + } + } + if (typeof validationResult === 'string') { fieldState.errorMessage = validationResult fieldState.valid = false - // TODO: this is unpredictable, need to figure out why - // It will sometimes lead to inconsistencies across re-renders - if (!parentErrorPaths.includes(`${path}${field.name}`)) { - parentErrorPaths.push(`${path}${field.name}`) - } + addErrorPathToParent(`${path}${field.name}`) } else { fieldState.valid = true } @@ -174,9 +182,9 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom acc.promises.push( iterateFields({ id, + addErrorPathToParent, anyParentLocalized: field.localized || anyParentLocalized, data: row, - errorPaths: fieldState.errorPaths, fields: field.fields, filter, forceFullValue, @@ -202,7 +210,6 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom collapsedRowIDs === undefined ? field.admin.initCollapsed : collapsedRowIDs.includes(row.id), - errorPaths: fieldState.errorPaths, }) return acc @@ -287,9 +294,9 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom acc.promises.push( iterateFields({ id, + addErrorPathToParent, anyParentLocalized: field.localized || anyParentLocalized, data: row, - errorPaths: fieldState.errorPaths, fields: block.fields, filter, forceFullValue, @@ -316,7 +323,6 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom collapsedRowIDs === undefined ? field.admin.initCollapsed : collapsedRowIDs.includes(row.id), - errorPaths: fieldState.errorPaths, }) } @@ -356,9 +362,9 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom case 'group': { await iterateFields({ id, + addErrorPathToParent, anyParentLocalized: field.localized || anyParentLocalized, data: data?.[field.name] || {}, - errorPaths: parentErrorPaths, fields: field.fields, filter, forceFullValue, @@ -491,9 +497,10 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom // Handle field types that do not use names (row, etc) await iterateFields({ id, + // passthrough parent functionality + addErrorPathToParent: addErrorPathToParentArg, anyParentLocalized: field.localized || anyParentLocalized, data, - errorPaths: parentErrorPaths, fields: field.fields, filter, forceFullValue, @@ -513,9 +520,10 @@ export const addFieldStatePromise = async (args: AddFieldStatePromiseArgs): Prom const promises = field.tabs.map((tab) => iterateFields({ id, + // passthrough parent functionality + addErrorPathToParent: addErrorPathToParentArg, anyParentLocalized: tab.localized || anyParentLocalized, data: tabHasName(tab) ? data?.[tab.name] : data, - errorPaths: parentErrorPaths, fields: tab.fields, filter, forceFullValue, diff --git a/packages/ui/src/forms/buildStateFromSchema/index.tsx b/packages/ui/src/forms/buildStateFromSchema/index.tsx index 74f6166261..211f3ac0b6 100644 --- a/packages/ui/src/forms/buildStateFromSchema/index.tsx +++ b/packages/ui/src/forms/buildStateFromSchema/index.tsx @@ -39,8 +39,8 @@ export const buildStateFromSchema = async (args: Args): Promise => { await iterateFields({ id, + addErrorPathToParent: null, data: fullData, - errorPaths: [], fields: fieldSchema, fullData, operation, diff --git a/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts b/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts index e7455f8d8f..7bbaf3146c 100644 --- a/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts +++ b/packages/ui/src/forms/buildStateFromSchema/iterateFields.ts @@ -7,12 +7,12 @@ import type { AddFieldStatePromiseArgs } from './addFieldStatePromise.js' import { addFieldStatePromise } from './addFieldStatePromise.js' type Args = { + addErrorPathToParent: (path: string) => void /** * if any parents is localized, then the field is localized. @default false */ anyParentLocalized?: boolean data: Data - errorPaths: string[] fields: FieldSchema[] filter?: (args: AddFieldStatePromiseArgs) => boolean /** @@ -58,9 +58,9 @@ type Args = { */ export const iterateFields = async ({ id, + addErrorPathToParent: addErrorPathToParentArg, anyParentLocalized = false, data, - errorPaths, fields, filter, forceFullValue = false, @@ -78,7 +78,7 @@ export const iterateFields = async ({ }: Args): Promise => { const promises = [] - fields.forEach((field) => { + fields.forEach((field, fieldIndex) => { if (!fieldIsPresentationalOnly(field) && !field?.admin?.disabled) { let passesCondition = true if (!skipConditionChecks) { @@ -92,10 +92,11 @@ export const iterateFields = async ({ promises.push( addFieldStatePromise({ id, + addErrorPathToParent: addErrorPathToParentArg, anyParentLocalized, data, - errorPaths, field, + fieldIndex, filter, forceFullValue, fullData, diff --git a/packages/ui/src/forms/useField/index.tsx b/packages/ui/src/forms/useField/index.tsx index 1710315783..59aa67954f 100644 --- a/packages/ui/src/forms/useField/index.tsx +++ b/packages/ui/src/forms/useField/index.tsx @@ -105,6 +105,7 @@ export const useField = (options: Options): FieldType => { const result: FieldType = useMemo( () => ({ errorMessage: field?.errorMessage, + errorPaths: field?.errorPaths || [], filterOptions, formProcessing: processing, formSubmitted: submitted, @@ -123,6 +124,7 @@ export const useField = (options: Options): FieldType => { field?.errorMessage, field?.rows, field?.valid, + field?.errorPaths, processing, setValue, showError, diff --git a/packages/ui/src/forms/useField/types.ts b/packages/ui/src/forms/useField/types.ts index bd7a53eff2..899898216d 100644 --- a/packages/ui/src/forms/useField/types.ts +++ b/packages/ui/src/forms/useField/types.ts @@ -12,6 +12,7 @@ export type Options = { export type FieldType = { errorMessage?: string + errorPaths?: string[] filterOptions?: FilterOptionsResult formProcessing: boolean formSubmitted: boolean