-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DataForm: migrate order action modal and introduce form validation #63895
Changes from all commits
d26735b
e469b7c
fc8a4fd
23ef641
33d24db
7e33991
f9cdc89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { isItemValid } from '../validation'; | ||
import type { Field } from '../types'; | ||
|
||
describe( 'validation', () => { | ||
it( 'fields not visible in form are not validated', () => { | ||
const item = { id: 1, valid_order: 2, invalid_order: 'd' }; | ||
const fields: Field< {} >[] = [ | ||
{ | ||
id: 'valid_order', | ||
type: 'integer', | ||
}, | ||
{ | ||
id: 'invalid_order', | ||
type: 'integer', | ||
}, | ||
]; | ||
const form = { visibleFields: [ 'valid_order' ] }; | ||
const result = isItemValid( item, fields, form ); | ||
expect( result ).toBe( true ); | ||
} ); | ||
|
||
it( 'integer field is valid if value is integer', () => { | ||
const item = { id: 1, order: 2, title: 'hi' }; | ||
const fields: Field< {} >[] = [ | ||
{ | ||
type: 'integer', | ||
id: 'order', | ||
}, | ||
]; | ||
const form = { visibleFields: [ 'order' ] }; | ||
const result = isItemValid( item, fields, form ); | ||
expect( result ).toBe( true ); | ||
} ); | ||
|
||
it( 'integer field is invalid if value is not integer', () => { | ||
const item = { id: 1, order: 'd' }; | ||
const fields: Field< {} >[] = [ | ||
{ | ||
id: 'order', | ||
type: 'integer', | ||
}, | ||
]; | ||
const form = { visibleFields: [ 'order' ] }; | ||
const result = isItemValid( item, fields, form ); | ||
expect( result ).toBe( false ); | ||
} ); | ||
|
||
it( 'integer field is invalid if value is empty', () => { | ||
const item = { id: 1, order: '' }; | ||
const fields: Field< {} >[] = [ | ||
{ | ||
id: 'order', | ||
type: 'integer', | ||
}, | ||
]; | ||
const form = { visibleFields: [ 'order' ] }; | ||
const result = isItemValid( item, fields, form ); | ||
expect( result ).toBe( false ); | ||
} ); | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { normalizeFields } from './normalize-fields'; | ||
import type { Field, Form } from './types'; | ||
|
||
export function isItemValid< Item >( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
item: Item, | ||
fields: Field< Item >[], | ||
form: Form | ||
): boolean { | ||
const _fields = normalizeFields( | ||
fields.filter( ( { id } ) => !! form.visibleFields?.includes( id ) ) | ||
); | ||
return _fields.every( ( field ) => { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
return false; | ||
} | ||
|
||
if ( | ||
field.type === 'integer' && | ||
! Number.isInteger( Number( value ) ) | ||
) { | ||
return false; | ||
} | ||
|
||
// Nothing to validate. | ||
return true; | ||
} ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,14 +11,13 @@ import { store as noticesStore } from '@wordpress/notices'; | |
import { useMemo, useState } from '@wordpress/element'; | ||
import { privateApis as patternsPrivateApis } from '@wordpress/patterns'; | ||
import { parse } from '@wordpress/blocks'; | ||
import { DataForm } from '@wordpress/dataviews'; | ||
import { DataForm, isItemValid } from '@wordpress/dataviews'; | ||
import { | ||
Button, | ||
TextControl, | ||
__experimentalText as Text, | ||
__experimentalHStack as HStack, | ||
__experimentalVStack as VStack, | ||
__experimentalNumberControl as NumberControl, | ||
} from '@wordpress/components'; | ||
|
||
/** | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #63849 is an attempt to address this but it's still unclear how to do it. |
||
const fields = [ | ||
{ | ||
type: 'text', | ||
header: __( 'Title' ), | ||
id: 'title', | ||
label: __( 'Title' ), | ||
placeholder: __( 'No title' ), | ||
getValue: ( { item } ) => item.title, | ||
}, | ||
{ | ||
type: 'integer', | ||
id: 'menu_order', | ||
label: __( 'Order' ), | ||
description: __( 'Determines the order of pages.' ), | ||
}, | ||
]; | ||
|
||
const form = { | ||
const formDuplicateAction = { | ||
visibleFields: [ 'title' ], | ||
}; | ||
|
||
const formOrderAction = { | ||
visibleFields: [ 'menu_order' ], | ||
}; | ||
|
||
/** | ||
* Check if a template is removable. | ||
* | ||
|
@@ -635,21 +644,20 @@ function useRenamePostAction( postType ) { | |
} | ||
|
||
function ReorderModal( { items, closeModal, onActionPerformed } ) { | ||
const [ item ] = items; | ||
const [ item, setItem ] = useState( items[ 0 ] ); | ||
const orderInput = item.menu_order; | ||
const { editEntityRecord, saveEditedEntityRecord } = | ||
useDispatch( coreStore ); | ||
const { createSuccessNotice, createErrorNotice } = | ||
useDispatch( noticesStore ); | ||
const [ orderInput, setOrderInput ] = useState( item.menu_order ); | ||
|
||
async function onOrder( event ) { | ||
event.preventDefault(); | ||
if ( | ||
! Number.isInteger( Number( orderInput ) ) || | ||
orderInput?.trim?.() === '' | ||
) { | ||
|
||
if ( ! isItemValid( item, fields, formOrderAction ) ) { | ||
return; | ||
} | ||
|
||
try { | ||
await editEntityRecord( 'postType', item.type, item.id, { | ||
menu_order: orderInput, | ||
|
@@ -673,9 +681,7 @@ function ReorderModal( { items, closeModal, onActionPerformed } ) { | |
} ); | ||
} | ||
} | ||
const saveIsDisabled = | ||
! Number.isInteger( Number( orderInput ) ) || | ||
orderInput?.trim?.() === ''; | ||
const isSaveDisabled = ! isItemValid( item, fields, formOrderAction ); | ||
return ( | ||
<form onSubmit={ onOrder }> | ||
<VStack spacing="5"> | ||
|
@@ -684,12 +690,11 @@ function ReorderModal( { items, closeModal, onActionPerformed } ) { | |
'Determines the order of pages. Pages with the same order value are sorted alphabetically. Negative order values are supported.' | ||
) } | ||
</div> | ||
<NumberControl | ||
__next40pxDefaultSize | ||
label={ __( 'Order' ) } | ||
help={ __( 'Set the page order.' ) } | ||
value={ orderInput } | ||
onChange={ setOrderInput } | ||
<DataForm | ||
data={ item } | ||
fields={ fields } | ||
form={ formOrderAction } | ||
onChange={ setItem } | ||
/> | ||
<HStack justify="right"> | ||
<Button | ||
|
@@ -706,7 +711,7 @@ function ReorderModal( { items, closeModal, onActionPerformed } ) { | |
variant="primary" | ||
type="submit" | ||
accessibleWhenDisabled | ||
disabled={ saveIsDisabled } | ||
disabled={ isSaveDisabled } | ||
__experimentalIsFocusable | ||
> | ||
{ __( 'Save' ) } | ||
|
@@ -873,7 +878,7 @@ const useDuplicatePostAction = ( postType ) => { | |
<DataForm | ||
data={ item } | ||
fields={ fields } | ||
form={ form } | ||
form={ formDuplicateAction } | ||
onChange={ setItem } | ||
/> | ||
<HStack spacing={ 2 } justify="end"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).