Add bulk editing support by passing array down to the fields #67584
Conversation
louwie17
added
[Type] Enhancement
packages/dataviews/src/types.ts
Outdated
| data: Item; | ||
| export type DataFormControlProps< | ||
| Item, | ||
| SupportsBulkEditing extends boolean = true, |
There was a problem hiding this comment.
I added this additional type to make it easier to handle within fields that don't support bulk editing, but maybe there is a better way?
There was a problem hiding this comment.
It is just a personal preference, but I would prefer type composition:
export type DataFormControlProps< Item > = {
data: Item;
field: NormalizedField< Item >;
onChange: ( value: Record< string, any > ) => void;
hideLabelFromVision?: boolean;
value: any;
};
export type WithBulkEditing< Item > = {
data: Item | Item[];
};And use in this way:
export const ParentEdit = ( {
data,
field,
onChange,
}: DataFormControlProps< BasePost > & WithBulkEditing< BasePost > ) => {We could do export already the composed type:
export type DataFormControlWithBulkEditingProps< Item > = DataFormControlProps< Item > & WithBulkEditing< Item >;
export const ParentEdit = ( {
data,
field,
onChange,
}: DataFormControlWithBulkEditingProps< BasePost >
| @@ -27,7 +27,7 @@ const SlugEdit = ( { | |||
| field, | |||
| onChange, | |||
| data, | |||
| }: DataFormControlProps< BasePost > ) => { | |||
| }: DataFormControlProps< BasePost, false > ) => { | |||
| const { id } = field; | |||
|
|
|||
| const slug = field.getValue( { item: data } ) || getSlug( data ); | |||
There was a problem hiding this comment.
If slug where to support bulk edits how would we handle the getSlug call here.
field.getValue( { item: data } ) can be replaced by the value prop.
|
Size Change: +565 B (+0.03%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
packages/dataviews/src/types.ts
Outdated
| data: Item; | ||
| export type DataFormControlProps< | ||
| Item, | ||
| SupportsBulkEditing extends boolean = true, |
There was a problem hiding this comment.
It is just a personal preference, but I would prefer type composition:
export type DataFormControlProps< Item > = {
data: Item;
field: NormalizedField< Item >;
onChange: ( value: Record< string, any > ) => void;
hideLabelFromVision?: boolean;
value: any;
};
export type WithBulkEditing< Item > = {
data: Item | Item[];
};And use in this way:
export const ParentEdit = ( {
data,
field,
onChange,
}: DataFormControlProps< BasePost > & WithBulkEditing< BasePost > ) => {We could do export already the composed type:
export type DataFormControlWithBulkEditingProps< Item > = DataFormControlProps< Item > & WithBulkEditing< Item >;
export const ParentEdit = ( {
data,
field,
onChange,
}: DataFormControlWithBulkEditingProps< BasePost > | const intersects = remainingRecords.every( ( item ) => { | ||
| return fieldDefinition.getValue( { item } ) === firstValue; | ||
| } ); | ||
| return intersects ? firstValue : MIXED_VALUE; |
There was a problem hiding this comment.
In regular layout we are passing the Symbol as value prop. I'm not sure that this is handled in the right way in the existent fields. For instance the feature image field:
| field, | ||
| onChange, | ||
| value, | ||
| }: DataFormControlProps< BasePost > ) => { |
There was a problem hiding this comment.
Featured Image doesn't support the bulk editing, should we set DataFormControlProps< BasePost, false >?
| @@ -64,6 +64,12 @@ export function normalizeFields< Item >( | |||
| ); | |||
| }; | |||
|
|
|||
| let supportsBulkEditing = true; | |||
| // If custom Edit component is passed in we default to false for bulk edit support. | |||
| if ( typeof field.Edit === 'function' || field.supportsBulkEditing ) { | |||
There was a problem hiding this comment.
I'm not sure about this logic.
I think that we need to decide whether bulk editing support should be opt-in or opt-out, regardless of whether the Edit function is present.
There was a problem hiding this comment.
I think that we need to decide whether bulk editing support should be opt-in or opt-out, regardless of whether the Edit function is present.
Yeah I agree! I would lean towards optOut
louwie17
force-pushed
the
add/65685_bulk_editing_support_second_approach
branch
from
0228bbc to
0f3c00c
Compare
| field, | ||
| onChange, | ||
| hideLabelFromVision, | ||
| }: DataFormControlProps< Item > ) { | ||
| value, | ||
| }: DataFormControlProps< Item > & WithBulkEditing< Item > ) { |
There was a problem hiding this comment.
I don't understand this change. maybe I'm missing something when reading the diff though. Is "data" optional? where does "value" come from? I mean where are these type changes defined?
There was a problem hiding this comment.
I updated the types.ts with the above: https://github.com/WordPress/gutenberg/pull/67584/files#diff-05cd2939bbf237a5dbcd116a0da9c9019b05ec1ceaa9d429c5fc872f149f63c4R192-R197
data is still passed in, but is either Item or Item | Item[] depending if combined with WithBulkEditing< Item >
There was a problem hiding this comment.
Do we still need data?
There was a problem hiding this comment.
Yeah we still need data as there are still fields that rely on other data. Like the parent field which makes use of the post id and post type ( from the data object ) to filter the parent list. Or the slug field that uses permalink_template and link properties as well.
Currently I don't display these fields in bulk edit mode, as the logic may change a bit if there are multiple records passed down.
Unless there is a better way to pass these additional values down.
| @@ -34,7 +33,7 @@ export default function DateTime< Item >( { | |||
| <VisuallyHidden as="legend">{ label }</VisuallyHidden> | |||
| ) } | |||
| <TimePicker | |||
| currentTime={ value } | |||
| currentTime={ typeof value === 'symbol' ? '' : value } | |||
There was a problem hiding this comment.
I don't see the check about symbol in other controls like integer, why?
There was a problem hiding this comment.
I will update it, the TimePicker component it-self seemed to handle it gracefully, but its not expected.
| return fieldDefinition.getValue( { | ||
| item: data, | ||
| } ); | ||
| }, [ data, fieldDefinition ] ); |
There was a problem hiding this comment.
If I'm not wrong this merging is probably duplicated in "regular" too. Should we extract it to a utility?
There was a problem hiding this comment.
Correct, I can move it to a utility 👍
| @@ -138,7 +157,13 @@ function PanelDropdown< Item >( { | |||
| ) } | |||
| onClick={ onToggle } | |||
| > | |||
| <fieldDefinition.render item={ data } /> | |||
| { showMixedValue ? ( | |||
| __( 'Mixed' ) | |||
There was a problem hiding this comment.
In one of the issues, @jasmussen mentions the possibility to render a different "mixed" label/component based on the "type" of the control. So potentially we could defer this to the control/field itself at some point.
There was a problem hiding this comment.
The context is loosely discussed here.
| @@ -76,6 +81,7 @@ export function normalizeFields< Item >( | |||
| sort, | |||
| isValid, | |||
| Edit, | |||
| supportsBulkEditing, | |||
There was a problem hiding this comment.
Should we mark supportsBulkEditing as required in the "NormalizedField" type?
|
Sharing some early thoughts after having given this a quick test (haven't looked at the code yet):
Screen.Recording.2024-12-10.at.11.57.54.mov |
| @@ -64,6 +64,11 @@ export function normalizeFields< Item >( | |||
| ); | |||
| }; | |||
|
|
|||
| const supportsBulkEditing = | |||
There was a problem hiding this comment.
Why bulk editing is tied to the field type or even having a custom edit function? In my view, this is business/consumer logic: users can't edit fields that are unique. That is unrelated to any field type or what kind of edit function is in place.
I was looking for something like this in the fields package instead (in the slug field, for example). Additionally, if we agree that the only case in which a field cannot be bulk edited is when it needs to be unique, I wonder how can we tie this to validation. For example, when editing the field, the user shouldn't be able to add a value that is in use by another item.
There was a problem hiding this comment.
The way I think about this is the following: any field is bulk editable regardless of their fieldType/edit function, unless it provides a specific opt-out (it's an unique field).
There was a problem hiding this comment.
Some fields could be unique for instance or things like that.
Thanks :) I will be making some updates after Riad's review.
Yeah good question, I think limiting to unique fields only makes sense and defaulting to fields supporting bulk editing. In terms of additional use cases, it may also be a question for @jameskoster. For example there is nothing limiting us from displaying the featured image field compared to the rest, but is this something users will actually use within bulk edit mode? This also brings up the question of whether we want to disable bulk editing purely within the layout ( although the field may support it ). Something @gigitux suggested in my last version to move the gutenberg/packages/dataviews/src/types.ts Lines 526 to 530 in 0f3c00c
Looks like I forgot to update this line with |
I see this panel as an Inspector/Edit interface, so it seems reasonably clear to me that we should show uneditable fields while inspecting a single record. I don't have a strong feeling about bulk editing though, it may be something we decide field by field. For instance 'file size' in the media library should show a range based on the selection, which might be useful, whereas 'File type: Mixed' is less useful. That said, it seems okay to hide uneditable fields during bulk selection for now :)
Prohibiting bulk edits to unique fields seems like a good place to start. I agree it's unlikely a user would want to use the same featured image, title, etc. on multiple records, but I also acknowledge it's not impossible, so I don't mind allowing that initially. |
|
Just pushed up a couple more changes, but not entirely ready for another round of reviews. |
|
Flaky tests detected in b9ca481. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12300375146
|
I agree with this direction. How do we distill that into API? As I mentioned above, what I'd expect to see here is something along the lines of an [
{
id: 'slug',
unique: true
}
]This new prop will control the bulk editing experience, and will also be used in validation as well (to be explored in a follow-up PR). We could introduce something specific for things that are not unique but want bulk editing disabled anyway, like a [
{
id: 'slug',
unique: true
},
{
id: 'feature_image',
bulkEditing: false
}
]I think I'd start by implementing the |
This is a great way to connect the dots! Thanks for proposing this! I'm entirely agree with this approach! |
…cking more robust
louwie17
force-pushed
the
add/65685_bulk_editing_support_second_approach
branch
from
f6e3ec4 to
6c629e6
Compare
| id | ||
| ) | ||
| ) | ||
| : null, |
There was a problem hiding this comment.
I will replace this with getEditedEntityRecords once #67872 is merged.
Yeah I am totally onboard with using the gutenberg/packages/dataviews/src/types.ts Lines 536 to 540 in b9ca481 Both places is also a valid answer :) |
Yes, what you did. |
|
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. |
|
@gigitux, @youknowriad, and @oandregal I moved this PR out of draft now and should be ready for a re-review. There is just one thing that is dependant on the hook introduced in this PR: #67872, that I will have to update once that is merged. |
|
Took a look at this one. Haven't been able to review all use cases (combined fields).
I wonder how can we improve the experience for consumers in the
Thinking out loud about 3. We don't really know anything about the data What if we just pass the array in the 3 scenario and we never pass |
Thanks appreciate it :)
Do note that for the gutenberg/packages/dataviews/src/types.ts Line 190 in 2e3e6e4
Yeah I agree, this is why I end up adding the
This approach would help simplify our logic and keep things consistent. It also forces 3rd party Edit functions to support "bulk editing" by default. Or are you thinking that we did still allow users to pass in an object to DataForm -> |
|
I'd like to do the minimal thing that would work so we can merge and iterate. These are my current thoughts:
As for
For this PR, I'd simplify by going with the second option. |
I would argue that for most situations, passing the "value" is the best thing to do. If you're a "string control" or a "number control" or whatever, you don't really care about the item or array of items, you just want to know the current value, or whether the value is mixed. I feel the second option (pass item or array) is actually more complex because it forces the |
What?
Allows consumers to pass in an array of items to be edited. The panel view will show "Mixed" when the values are different across the items.
Why?
Partially addresses: #65685
The header for bulk editing will be addressed in a separate PR.
How?
This is an alternative solution to: #67344
DataForm allows an array of items to be passed in.
When an array of items is passed in it does a couple things:
dataprop now supportsItemandItem[]rendermethod receives avalueprop which can be the Mixed symbol.getValueand pass the value down. They also set it to Mixed if bulk editing and the value isn't the same across all records.Some questions/follow up:
rendermethod takes a singleitemprop, how do we handle this when passing in an array of items? Should we replace this with thedataandvalueprops similar to theeditmethod? Or add support for anitemsprop?Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
bulk-editing.mp4