[WIP/Exploration]: DataForm validation/form field state #67357
Conversation
|
Size Change: +1.73 kB (+0.09%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
gigitux
changed the title
gigitux
changed the base branch from
trunk
to
add/use-data-form-pattern-creation
| hideLabelFromVision={ hideLabelFromVision } | ||
| /> | ||
| { errorMessage && ( | ||
| <p className="dataform-control-error">{ errorMessage }</p> |
There was a problem hiding this comment.
There was a problem hiding this comment.
Just FYI: I think our components friends would prefer a dedicated PR for this :)
There was a problem hiding this comment.
Yeah, it is already in my plan. This is mostly a POC. Happy to help! 💪
| fields: supportedFields, | ||
| touchedFields: [] as string[], | ||
| messageErrors: {}, | ||
| } ); |
There was a problem hiding this comment.
Feels like a bit of a complex state, useReducer might prove to be better in the long term.
| setTouchedFields, | ||
| setErrors, | ||
| isFormValid, | ||
| }; |
There was a problem hiding this comment.
Some of these are used as dependencies in hooks later... so probably better to memoize (useEvent/useCallback)
| @@ -28,6 +28,7 @@ export function DataFormLayout< Item >( { | |||
| field: FormField; | |||
| onChange: ( value: any ) => void; | |||
| hideLabelFromVision?: boolean; | |||
| errorMessage: string | undefined; | |||
There was a problem hiding this comment.
I'm not sure if I see this as a "field" property or a "field layout" property.
packages/dataviews/src/types.ts
Outdated
| messageErrors: Record< string, string | undefined >; | ||
| setTouchedFields: ( touchedFields: string[] ) => void; | ||
| setErrors: ( field: string, error: string | undefined ) => void; | ||
| isFormValid: ( data: Record< string, any > ) => boolean; |
There was a problem hiding this comment.
I think this is mixing several things together. form for me is about the "layout" of the form, how do we render the form. I think validation state should probably be separate from it. Maybe the current form prop name is what pushed you to add this stuff here and we might want to consider renaming it "view" or something like that.
There was a problem hiding this comment.
I don't have a strong opinion. IMHO form could be a big object with layout and validation option or spread them across multiple props.
| title: { | ||
| validation: { | ||
| showErrorOnlyWhenDirty: true, | ||
| callback: ( data ) => { |
There was a problem hiding this comment.
I feel this function should ideally be "absorbed" in a declarative way ultimately by having things like minLengh, maxLength, isRequired... in the field definition maybe.
There was a problem hiding this comment.
This means that what I wrote in the PR description isn't completely true:
The primary consideration is that fields should contain only UI logic, while business logic should reside in the consumer codebase. Consequently, the isValid callback should not be implemented at the field level.
I would avoid implementing validation on the field level, mostly I feel that different consumers could need different validation.
EDIT: Mmmm, I think that there is a nuance here. I feel that it makes sense that @wordpress/fields are opinionated, but I can imagine that consumers would like to override validation rules.
Should we support validation on both side:
- field level
- consumer level
?
There was a problem hiding this comment.
Regarding the “declarative way,” we should support a generic function, allowing consumers to choose their preferred validation library.
However, we could use a declarative approach once Gutenberg adopts its own validation library (see: #67324).
Additionally, it’s important to note that implementing validation at the field level restricts validation to that specific field. It’s common to encounter scenarios where a field’s valid status depends on the value of another field.
There was a problem hiding this comment.
There are two questions:
- Should it be field level, for and given the discussion in Proposal: JSON Schema validation in Gutenberg #67324 and the relationship with JSON schema, I think the field level would have my preference for now. We probably need more advanced multi-field validation later but that's for another day :P
- Should we start by the declarative approach or the escape hatch approach. I think for me this is still unclear to answer right now. If we go declarative, we need to try to match JSON schema. The con is it involves a bit more "architecture" work on the framework side.
There was a problem hiding this comment.
Doesn't set validation at the field level prevents consumers from applying custom validation to generic fields like “text” or “date”? They will need to create custom fields instead.
There was a problem hiding this comment.
We probably need more advanced multi-field validation later but that's for another day :P
How would that look? Schema (with $data) already supports multiple fields. All client side validation relates to a field in the form, so ultimately, all errors should point to the field that caused the error.
Errors that can't be linked to a field probably come from server or before reaching the form, and those might be related to the form UI I think.
There was a problem hiding this comment.
Doesn't set validation at the field level prevents consumers from applying custom validation to generic fields like “text” or “date”? They will need to create custom fields instead.
I don't understand how it would prevent anything? Here's a rough idea how I see things
const fields = [ {
id: 'title',
type: 'string',
minLength: 10,
maxLength: 50,
isRequired: true,
} ]There was a problem hiding this comment.
Errors that can't be linked to a field probably come from server or before reaching the form, and those might be related to the form UI I think.
I don't know yet how it would look, I need use-cases to be able to think about it first but yeah I think forms can probably have global errors that are not directly attached to fields
There was a problem hiding this comment.
I don't understand how it would prevent anything? Here's a rough idea how I see things
Oooh, ok. I was convicted that you were referring to putting validation constraint in the field declaration. Sorry for the confusion.
We could follow this approach, especially given the discussion in WordPress Gutenberg #67324. However, we should also allow users to define custom callbacks if they prefer a different validation library or need to perform complex validations, such as multi-field validation.
The key point is to define the validation state on the consumer side and pass it to the dataform. This ensures that both the library and the consumer can update the state as needed.
There was a problem hiding this comment.
Oooh, ok. I was convicted that you were referring to putting validation constraint in the field declaration. Sorry for the confusion.
Yes, that example is a field.
We could follow this approach, especially given the discussion in #67324. However, we should also allow users to define custom callbacks if they prefer a different validation library or need to perform complex validations, such as multi-field validation.
Maybe, but my main point is that this is an edge case and we should treat it once we face it as otherwise, I'm not sure we should be providing the right solution.
youknowriad
added
[Type] Experimental
|
Thanks for your precious feedback, @youknowriad! 🙇 |
|
@gigitux is there any reason we're coupling the form this early? I do agree with you that form fields and form in general only worry about UI but we're now passing validation functions, which I believe is too much coupling? I believe it was discussed before but shouldn't you run validation outside, assuming the form state can be accessed outside, you should just do whatever logic you want and return an error object of field id and errors. This allows client side validation, server validation, and whatever logic you want. In your case, you cannot for example hydrate the form with existing errors if needed (if it already has values), since you need to run validate on touch/blur. There's also no way to mark a field invalid from outside the validation function (which has its runtime controlled by the dataform), this means you can't trigger errors from other places. |
You can do it! async function onCreate(patternTitle, sync) {
try {
setIsSaving(true);
const categories = await Promise.all(
pattern.categoryTerms.map((termName) => findOrCreateTerm(termName))
);
const newPattern = await createPattern(
patternTitle,
sync,
typeof content === "function" ? content() : content,
categories
);
onSuccess({
pattern: newPattern,
categoryId: PATTERN_DEFAULT_CATEGORY,
});
} catch (error) {
createErrorNotice(error.message, {
type: "snackbar",
id: "pattern-create",
});
form.setErrors("title", "Backend Error: This");
}
}Screen.Capture.on.2024-12-02.at.12-34-41.mp4
We can do it, I didn't implement these kinds of utils mostly because it is just a POC. |
…berg into add/handle-message-error
gigitux
changed the base branch from
add/use-data-form-pattern-creation
to
trunk
gigitux
changed the base branch from
trunk
to
add/use-data-form-pattern-creation
| value.length >= schema.maxLength ? 'Length is too long' : undefined; | ||
| } | ||
|
|
||
| return callbacks; |
There was a problem hiding this comment.
This should be part of each "Field type" somehow, each field type has its own "config/constraints"
There was a problem hiding this comment.
Still not ready for another review! I'm cleaning up the PR and writing another message to explain last changes.
There was a problem hiding this comment.
This should be part of each "Field type" somehow, each field type has its own "config/constraints"
I'm not sure that I understood this comment, but each field will have a `validationSchema" property:
gutenberg/packages/dataviews/src/types.ts
Lines 157 to 164 in 8b69e13
This "schema" will be parsed internally and transform this rules in callbacks that will be invoked to run the validation.
In the next hour, I will write a message to explain the changes that I did (I will tag you)
There was a problem hiding this comment.
We have field types "number", "text" ... and each field type should define a validation logic for it.
- number should support "min"/"max" constraints".
- string should support "minLenght"/"maxLength".
- isRequired is a constraint that works for all field types.
What I'm saying is that I feel this logic should be provided by the field types and not some global function like this.
|
I collect your feedback and try to address in this PR. Validation schema on the field levelThe validation schema is at the field level. The current shape of the Consumers can decide when to show the message error via the useValidation hookBy default, validation works smoothly with the validationSchema, but consumers might need more control over the form validation state. To address this, the This enhancement enables consumers to:
Further utilities are expected to be added to this hook in the future. cc @youknowriad |
…ress/gutenberg into add/handle-message-error
gigitux
changed the base branch from
add/use-data-form-pattern-creation
to
trunk
gigitux
changed the base branch from
trunk
to
add/use-data-form-pattern-creation
…ress/gutenberg into add/handle-message-error
| @@ -47,6 +49,34 @@ export function DataFormLayout< Item >( { | |||
| [ form ] | |||
| ); | |||
|
|
|||
| const firstValidationRunRef = useRef( false ); | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
Why do we need to trigger validation in an effect and not as an event handler?
There was a problem hiding this comment.
This runs the validation when the component is mounted. In the case of the pattern modal, we want to disable the button if the form isn't valid:
Screen.Capture.on.2024-12-05.at.11-28-03.mp4
The validation runs on the onChange event too:
gutenberg/packages/dataviews/src/dataforms-layouts/data-form-layout.tsx
Lines 123 to 152 in 03e934b
There was a problem hiding this comment.
Could we maybe also move this logic and trigger it in the DataForm component maybe?
The DataFormLayout is also being used within the panel layout, meaning every time the dropdown is opened this logic gets run again.
| ? Object.values( | ||
| validation.errorMessages[ formField.id ] ?? [] | ||
| )[ 0 ] | ||
| : ''; |
There was a problem hiding this comment.
What is this doing and is it the right place for this logic?
There was a problem hiding this comment.
The errorMessages dictionary contains an array of error messages for each field ID. This allows a field to have multiple error messages, such as those for password requirements.
Currently, only the first error message is passed to the field.
There was a problem hiding this comment.
I would also lean towards moving this logic to another place, could this logic be handled within the DataFormProvider or the useValidation hook?
Also I would write this out to an if statement, this is rather difficult to read.
| }; | ||
| }, {} ); | ||
|
|
||
| validation.setErrors( formField.id, errors ); |
There was a problem hiding this comment.
Do we need two callbacks for onChange and setErrors. There's something that's bothering me here:
The questions are:
- Should the validation be performed by
DataFormor by the consumer ofDataFormin theonChangeevent handler? (I don't know) - If the former, I think we should have
errorsas an extra argument of theonChangehandler maybe - It might be better to start with the latter though (validation performed on demand)
I feel like we need a good story here of what we want to achieve, it's still not clear to me.
There was a problem hiding this comment.
I think that the validation should be performed by DataForm, but allow consumers to run their custom logic in their codebase and set errors via the validation object returned by useValidation. Although the extensibility API is still being defined, one of our goals is to allow PHP developers to add or remove fields using only PHP code (from what I understood). If validation is implemented on the consumer side, it will complicate the process for them.
There was a problem hiding this comment.
It might be better to start with the latter though (validation performed on demand)
Thinking about this more, I would lean towards this. As I am also not entirely settled with the fact that onChange is run first and than the validation/touched states are updated.
While this may work for our current use case, I could see consumers not wanting to update the specific state if there was errors. With this they have no way of knowing.
Passing errors as the additional argument for onChange would fix this.
There was a problem hiding this comment.
Passing errors as the additional argument for onChange would fix this.
I'm going to work on another PR exploring this approach.
Just to clarify, are you imaging an API like this?
<DataForm
data={pattern}
fields={fields}
form={form}
onChange={(newData, errors) => {
const newErros = validateForm(newData, fields);
setPattern({
...pattern,
...newData,
});
errors.setErrors(newErrors);
}}
/>
<Button disabled={!isValidForm}>Save</Button>
Few thoughts:
- On-demand validation prevents marking the form as invalid on the first render. For instance, a form with required fields should be initially invalid, while a form without required fields could be valid from the start.
- Shifting validation solely to the consumer side means we can't ensure that a field with some constraints defined in the field definition will always undergo validation.
There was a problem hiding this comment.
Just to clarify, are you imaging an API like this?
Thanks for the example, I was actually thinking of keeping the validation triggered within the DataForm component, but that the onChange callback includes the errors ( similar to yours above ):
onChange={(newData, newErrors) => {
setPattern({
...pattern,
...newData,
});
errors.setErrors(newErrors);
}}
| validationSchema?: { | ||
| minLength: number; | ||
| maxLength: number; | ||
| onTouched: boolean; |
There was a problem hiding this comment.
what is this boolean about? (it feels misnamed)
There was a problem hiding this comment.
The naming could be improved. Essentially, this boolean indicates whether the error message should be displayed only when the field is dirty or also on the first validation run.
| onTouched set to false | onTouched set to true |
|---|---|
Screen.Capture.on.2024-12-05.at.11-20-34.mp4 |
Screen.Capture.on.2024-12-05.at.11-21-30.mov |
| /** | ||
| * Validation schema for the field. | ||
| */ | ||
| validationSchema?: { |
There was a problem hiding this comment.
Do we need this top level thing? It seems this is moving us away from JSON schema "format" where the field name, id, type... are mixed with the specific constraints like "minLength"... so maybe we can just move the constraints top level (but make them dependent on the field type for most of them)
There was a problem hiding this comment.
I don’t have a strong preference on this 👍. However, there may be some options that don’t belong in the JSON schema, such as the onTouched property.
…ress/gutenberg into add/handle-message-error
| if ( firstValidationRunRef.current ) { | ||
| return; | ||
| } | ||
| fieldDefinitions.forEach( ( fieldDefinition ) => { |
There was a problem hiding this comment.
Is there a reason you decided to run the validation against all the fieldDefinitions versus the normalizedFormFields ( the fields being rendered ).
This probably overlaps a 100% most of the cases, but for our bulk editing for example, not all the fields get rendered, but the same field definitions are still being passed in. Meaning it would still run validation for the hidden fields.
There was a problem hiding this comment.
Interesting point. I think that you're right! It doesn't make sense to run validation for all fields 👍
| @@ -47,6 +49,34 @@ export function DataFormLayout< Item >( { | |||
| [ form ] | |||
| ); | |||
|
|
|||
| const firstValidationRunRef = useRef( false ); | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
Could we maybe also move this logic and trigger it in the DataForm component maybe?
The DataFormLayout is also being used within the panel layout, meaning every time the dropdown is opened this logic gets run again.
| ? Object.values( | ||
| validation.errorMessages[ formField.id ] ?? [] | ||
| )[ 0 ] | ||
| : ''; |
There was a problem hiding this comment.
I would also lean towards moving this logic to another place, could this logic be handled within the DataFormProvider or the useValidation hook?
Also I would write this out to an if statement, this is rather difficult to read.
| }; | ||
| }, {} ); | ||
|
|
||
| validation.setErrors( formField.id, errors ); |
There was a problem hiding this comment.
It might be better to start with the latter though (validation performed on demand)
Thinking about this more, I would lean towards this. As I am also not entirely settled with the fact that onChange is run first and than the validation/touched states are updated.
While this may work for our current use case, I could see consumers not wanting to update the specific state if there was errors. With this they have no way of knowing.
Passing errors as the additional argument for onChange would fix this.
| onChange={ onChange } | ||
| errorMessage={ errorMessage } | ||
| onChange={ ( value ) => { | ||
| onChange( value ); |
There was a problem hiding this comment.
Could we move the logic below to a helper function?
Also does this still work within a Panel layout given we pass in children there?
There was a problem hiding this comment.
Could we maybe also move this logic and trigger it in the DataForm component maybe?
The DataFormLayout is also being used within the panel layout, meaning every time the dropdown is opened this logic gets run again.
Good point, you're right! I will double-check this!
There was a problem hiding this comment.
While this may work for our current use case, I could see consumers not wanting to update the specific state if there was errors. With this they have no way of knowing.
Just to clarify, for instance, you're referring to a field that can render max 5 characters in the input, right? If yes, I think that kind of “constraints” is a UI thing, and it should be handled on the field level.
I think that validation should run on the last “snapshot” of the form values.
There was a problem hiding this comment.
Just to clarify, for instance, you're referring to a field that can render max 5 characters in the input, right?
Yeah, or an email/link field. But you're right, it should be a UI thing. And thinking about it more, it would be kinda strange if someone triggered an update with the last "valid" state, although a user kept typing in an invalid state.
| validationSchema: { | ||
| minLength: 1, | ||
| maxLength: 10, | ||
| onTouched: true, |
There was a problem hiding this comment.
In relation to @youknowriad comment above, it feels a bit weird to me to add these constraints as part of the field definition. Especially if we may use some of these same definitions across post types. For example would title validation be the same across a pattern, template, or category?
I did prefer to have something like validationSupports for a specific field definition or field type ( if needed, it may be irrelevant unless we make use of some of the native validation ).
I would lean towards something similar to the JSON schema, where we link the validation conditions to the key. In this case we may pass something like this to the useValidation hook:
{
"title": {
"minLength": 1,
"maxLength": 10,
"onTouched": true
}
}
The only questionable one being onTouched, that may be something better suited for the field definition.
There was a problem hiding this comment.
Foe me maxLength and minLength are not just validation constraints, they are tools to understand the field itself, the UI to edit it... They should be part of the field definition.
There was a problem hiding this comment.
@gigitux just pointed out this previous iteration and conversation, where the above approach was suggested, my bad 🙈
Foe me maxLength and minLength are not just validation constraints, they are tools to understand the field itself, the UI to edit it... They should be part of the field definition.
Gotcha, yeah I was still coming from the mindset of keeping the validation logic separate and allow the fields to just handle it visually.
But this in theory is more something for the field types, instead of the specific field definitions that are linked to a key anyway.
Related #59745
What?
This PR presents a proof of concept for handling error messages and validation in Dataform.
Validation schema on the field level
The validation schema is at the field level. The current shape of the
validationSchemais a POC, but the idea is that we will use adopt the same structure used by #67324.Consumers can decide when to show the message error via the
onTouchedvalue.useValidation hook
By default, validation works smoothly with the validationSchema, but consumers might need more control over the form validation state. To address this, the
useValidationhook can now be used, allowing consumers to pass a validation object to the dataform component.This enhancement enables consumers to:
Old approach:
For this reason, I created a
useFormhook. This hook allows consumers to define business logic of their form:gutenberg/packages/patterns/src/components/create-pattern-modal.js
Lines 77 to 103 in 46d6832
useFormhook returns an object. This object needs to be passed to theDataFormcomponent as propform.gutenberg/packages/patterns/src/components/create-pattern-modal.js
Lines 194 to 204 in 46d6832
This hook returns utilities and state for advanced logic. However, for simple use cases, dataform will take care of everything based on the validation object passed to the hook. For each id, the consumer can specify:
showErrorOnlyWhenDirty-> the error message will be passed to the layout ONLY when the input has been touched.callback-> you can return the validity of the field and the error message to render if the validation doesn't pass.gutenberg/packages/patterns/src/components/create-pattern-modal.js
Lines 79 to 103 in e01fb7f
Furthermore, the consumer can compute the current validity of the form using the
isFormValidfunction.gutenberg/packages/patterns/src/components/create-pattern-modal.js
Lines 223 to 224 in 7b8acfa
Field
The field receives an additional prop called
errorMessage, which can be either astringorundefined. Based on this value, the field can render an error message.https://github.com/WordPress/gutenberg/blob/e01fb7f7aec7990ae6aabba0f574bfbabab35ad5/packages/dataviews/src/dataform-controls/text.tsx#L12-L47
Demo
Screen.Capture.on.2024-11-28.at.15-10-45.mp4
Note
API names are still WIP. This PR is mostly to gathering feedback and explore more this approach with more complex use-case.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast