Skip to content
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.

Sign up for GitHub

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: add author to quick edit page/post list #63983

Merged
merged 9 commits into from
Jul 29, 2024
+76 −7
Merged

DataForm: add author to quick edit page/post list #63983

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
271dac7
Add validation to quick edit form
oandregal Jul 26, 2024
23d3904
Add author to quick-edit form
oandregal Jul 26, 2024
16cda97
Display select control for users
oandregal Jul 26, 2024
d88059f
Allow for undefined author
oandregal Jul 26, 2024
216234e
Bring back getValue for author
oandregal Jul 26, 2024
f3053ac
If elements are provided, the value needs to be one of them
oandregal Jul 26, 2024
1dff635
Update storybook
oandregal Jul 26, 2024
9bfa200
Remove unnecessary getValue from author field
oandregal Jul 29, 2024
70e6966
Convert to number before validating
oandregal Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions packages/dataviews/src/components/dataform/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import type { Dispatch, SetStateAction } from 'react';
import {
TextControl,
__experimentalNumberControl as NumberControl,
SelectControl,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useCallback, useMemo } from '@wordpress/element';

/**
Expand Down Expand Up @@ -65,8 +67,7 @@ function DataFormNumberControl< Item >( {
onChange,
}: DataFormControlProps< Item > ) {
const { id, label, description } = field;
const value = field.getValue( { item: data } );

const value = field.getValue( { item: data } ) ?? '';
const onChangeControl = useCallback(
( newValue: string | undefined ) =>
onChange( ( prevItem: Item ) => ( {
Expand All @@ -76,6 +77,29 @@ function DataFormNumberControl< Item >( {
[ id, onChange ]
);

if ( field.elements ) {
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
const elements = [
/*
* Value can be undefined when:
*
* - the field is not required
* - in bulk editing
*
*/
{ label: __( 'Select item' ), value: '' },
...field.elements,
];

return (
<SelectControl
label={ label }
value={ value }
options={ elements }
onChange={ onChangeControl }
/>
);
}

return (
<NumberControl
label={ label }
Expand Down
12 changes: 11 additions & 1 deletion packages/dataviews/src/components/dataform/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,26 @@ const fields = [
label: 'Order',
type: 'integer' as const,
},
{
id: 'author',
label: 'Author',
type: 'integer' as const,
elements: [
{ value: 1, label: 'Jane' },
{ value: 2, label: 'John' },
],
},
];

export const Default = () => {
const [ post, setPost ] = useState( {
title: 'Hello, World!',
order: 2,
author: 1,
} );

const form = {
visibleFields: [ 'title', 'order' ],
visibleFields: [ 'title', 'order', 'author' ],
};

return (
Expand Down
17 changes: 17 additions & 0 deletions packages/dataviews/src/test/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,21 @@ describe( 'validation', () => {
const result = isItemValid( item, fields, form );
expect( result ).toBe( false );
} );

it( 'field is invalid if value is not one of the elements', () => {
const item = { id: 1, author: 3 };
const fields: Field< {} >[] = [
{
id: 'author',
type: 'integer',
elements: [
{ value: 1, label: 'Jane' },
{ value: 2, label: 'John' },
],
},
];
const form = { visibleFields: [ 'author' ] };
const result = isItemValid( item, fields, form );
expect( result ).toBe( false );
} );
} );
7 changes: 7 additions & 0 deletions packages/dataviews/src/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ export function isItemValid< Item >(
return false;
}

if ( field.elements ) {
const validValues = field.elements.map( ( f ) => f.value );
if ( ! validValues.includes( value ) ) {
return false;
}
}

// Nothing to validate.
return true;
} );
Expand Down
17 changes: 14 additions & 3 deletions packages/edit-site/src/components/post-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import clsx from 'clsx';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { DataForm } from '@wordpress/dataviews';
import { DataForm, isItemValid } from '@wordpress/dataviews';
import { useDispatch, useSelect, useRegistry } from '@wordpress/data';
import { store as coreDataStore } from '@wordpress/core-data';
import { Button } from '@wordpress/components';
Expand Down Expand Up @@ -42,7 +42,7 @@ function PostEditForm( { postType, postId } ) {
const { saveEntityRecord } = useDispatch( coreDataStore );
const { fields } = usePostFields();
const form = {
visibleFields: [ 'title' ],
visibleFields: [ 'title', 'author' ],
};
const [ edits, setEdits ] = useState( {} );
const itemWithEdits = useMemo( () => {
Expand All @@ -53,6 +53,11 @@ function PostEditForm( { postType, postId } ) {
}, [ initialEdits, edits ] );
const onSubmit = async ( event ) => {
event.preventDefault();

if ( ! isItemValid( itemWithEdits, fields, form ) ) {
return;
}

const { getEntityRecord } = registry.resolveSelect( coreDataStore );
for ( const id of ids ) {
const item = await getEntityRecord( 'postType', postType, id );
Expand All @@ -64,6 +69,7 @@ function PostEditForm( { postType, postId } ) {
setEdits( {} );
};

const isUpdateDisabled = ! isItemValid( itemWithEdits, fields, form );
return (
<form onSubmit={ onSubmit }>
<DataForm
Expand All @@ -72,7 +78,12 @@ function PostEditForm( { postType, postId } ) {
form={ form }
onChange={ setEdits }
/>
<Button variant="primary" type="submit">
<Button
variant="primary"
type="submit"
accessibleWhenDisabled
disabled={ isUpdateDisabled }
>
{ __( 'Update' ) }
</Button>
</form>
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/src/components/post-fields/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ function usePostFields( viewType ) {
{
label: __( 'Author' ),
id: 'author',
type: 'integer',
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
getValue: ( { item } ) => item._embedded?.author[ 0 ]?.name,
Copy link
Member Author

@oandregal oandregal Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I'm aware, we didn't really use this function. The implementation is also wrong, because the getValue should return an integer (what item.author is) instead of a string (the display name is a render concern).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have a filter that uses this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary for filters, using the integer would be enough. However! Your comment makes me realize this is actually necessary for sorting: we should sort authors by name not by the id assigned by WordPress.

I'm going to bring back this, but sorting is not working on trunk. We need to look at that separately. There's a few issues (author & dates not sorting properly, the sort config in the table headers is not cleared properly):

Gravacao.do.ecra.2024-07-26.as.13.23.26.mov

Copy link
Member Author

@oandregal oandregal Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually raises an interesting question: if we have types but sorting depends on getValue, we can end up with the following (like in this case for author):

  • type is integer
  • but getValue returns a string

How do we make the sort utility work based on types in this scenario?

It seems we shouldn't use getValue for sorting and actually provide a custom sort utility if the field needs it. In that case, the filterSort utility would use the custom sort if it exists, otherwise would default to the sort defined by the field type. Thoughts?

(nothing to do in this PR, as I brought back the getValue function 216234e but wanted to share for consideration when looking at fixing sorting and formalizing types)

Copy link
Member Author

@oandregal oandregal Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked a bit more into this and it turns out getValue is not used for sorting either: we delegate on the REST API for filter/sort/rest in this page.

The issues I've seen with sorting in trunk are:

  • author is sorted by creation id, not by name. In the existing wp-admin page, users cannot sort by author.
  • date: I'm not sure why it doesn't work but it's unrelated. In the existing wp-admin page, this works fine.

I removed the getValue to proceed with this PR.

elements:
authors?.map( ( { id, name } ) => ( {
Expand Down
Loading