DataForm: migrate order action modal and introduce form validation #63895
Conversation
This file contains 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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
oandregal
changed the title
oandregal
added
[Type] Enhancement
oandregal
commented
Jul 24, 2024
| @@ -39,21 +38,31 @@ import { getItemTitle } from '../../dataviews/actions/utils'; | |||
| const { PATTERN_TYPES, CreatePatternModalContents, useDuplicatePatternProps } = | |||
| unlock( patternsPrivateApis ); | |||
|
|
|||
| // TODO: this should be shared with other components (page-pages). | |||
| // TODO: this should be shared with other components (see post-fields in edit-site). | |||
There was a problem hiding this comment.
#63849 is an attempt to address this but it's still unclear how to do it.
oandregal
changed the title
oandregal
commented
Jul 24, 2024
| import { normalizeFields } from './normalize-fields'; | ||
| import type { Field, Form } from './types'; | ||
|
|
||
| export function isItemValid< Item >( |
There was a problem hiding this comment.
This method just ports the existing logic into the new place, there's no behavioral changes.
In follow-up PRs we should discuss all the validation details iteratively: how a form declares the required fields (or is it the field itself?), etc. Those conversations shouldn't block this PR from landing.
|
Size Change: +86 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jul 24, 2024
There was a problem hiding this comment.
I like two things here:
- Validation is opt-in.
- We're starting small.
Some things that could be a bit better or follow-ups:
- Formalize field types definitions.
- Potentially start having a storybook story per field type
- We need to thing about how to test these things properly.
- I think we should focus more on the "quick edit" forms rather than these small existing forms. (Bring value to the user)
- Error handling is not clear either at this point.
I believe we can ship this though as a first step but as follow-ups for now, I'd give more priority to quick edit and trying to replicate the "document inspector" in the quick edit panels.
| const value = field.getValue( { item } ); | ||
|
|
||
| // TODO: this implicitely means the value is required. | ||
| if ( field.type === 'integer' && value === '' ) { |
There was a problem hiding this comment.
I think we need to formalize the field "types" better. I see us having a folder of field types and each field type would export a component to render the value, a component to render the control and a validation function. (Rather that having this logic within the global validation function)
oandregal
force-pushed
the
migrate/order-action-to-dataform
branch
from
bc4b99d
to
f9cdc89
Compare
This is fair, I'll give quick edit a push. Though having more than one use case is proving useful and has already raised many unanswered API questions: how to share field definition between edit-site and edit-post? is "required field" a form API or a field API?, etc. |
I've asked for design input at #63624 (comment) |
|
@youknowriad do you want this PR to do something else or do we ship and iterate? |
|
I'm ok shipping this 👍 I'll give it a final review. |
youknowriad
reviewed
Jul 26, 2024
| @@ -44,7 +44,7 @@ export type Operator = | |||
|
|
|||
| export type ItemRecord = Record< string, unknown >; | |||
|
|
|||
| export type FieldType = 'text'; | |||
| export type FieldType = 'text' | 'integer'; | |||
There was a problem hiding this comment.
I wonder if these should be 'text' | 'integer' or 'string' or 'number' to stay as close as possible to JSON schemas.
There was a problem hiding this comment.
I went for integer because that's the type for menu_order according to the REST API https://developer.wordpress.org/rest-api/reference/pages/ (in terms of validation it's also more fine-grained and different than number).
youknowriad
approved these changes
Jul 26, 2024
|
#63983 builds on this PR (type integer) to add support for "author" in the quick edit form. |
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.
Part of #59745
What?
This PR migrates the "Change order" action modal to use DataForm. It also prepares the scaffold for form validation.
How?
DataForm5dd8e8e.isItemValid( item, fields )to centralize validation 16f9f08.Testing Instructions