Dataform bulk editing support #67344
Conversation
| ( fieldDefinition ) => fieldDefinition.id === fieldId | ||
| )?.supportsBulk; | ||
| } ); | ||
| } |
There was a problem hiding this comment.
Checks if 'any' of the nested fields support bulk, returns 'true' in that case.
| @@ -111,6 +113,10 @@ function PanelDropdown< Item >( { | |||
| [ popoverAnchor ] | |||
| ); | |||
|
|
|||
| const fieldValue = fieldDefinition.getValue( { item: data } ); | |||
| const showMixedValue = | |||
| isBulkEditing && ( fieldValue === undefined || fieldValue === '' ); | |||
There was a problem hiding this comment.
This is where having a central intersected object falls short, and why I am leaning towards moving the array of items down so the field layout has more control and also optionally the field.
As in this case the getValue for title, returns an empty string if it isn't defined. While empty string may be a valid case for another field and different from undefined or null.
There was a problem hiding this comment.
yeah value undefined or '' might be valid values, this is why I suggested the "Symbol"
There was a problem hiding this comment.
@youknowriad Where was it that you suggested Symbol?
And are you thinking we did use it to define the Mixed value. That in the getIntersectingValues function for example when it is a mixed value we set it to Symbol.for( 'unique dataform mixed value key' )? and than check for that in the above? or are you thinking something else?
I did try the above, but ran into this error Cannot use 'in' operator to search for 'rendered' in Symbol(DATAFORM_MIXED_VALUE) within the getItemTitle function. So in order to make this work I did probably have to do what I suggested above:
I am leaning towards pushing the intersecting logic down to the field view and rely on getValue, but this will cascade into quite a few other changes.
There was a problem hiding this comment.
That in the getIntersectingValues function for example when it is a mixed value we set it to Symbol.for( 'unique dataform mixed value key' )? and than check for that in the above? or are you thinking something else?
yes
I'm not sure we should push the merging down to the "field". We should just check where getItemTitle is called and not call it when it's a symbol (I'm assuming it's an internal call to data form component)
There was a problem hiding this comment.
I just saw that some of my comments were not in the right place.
There was a problem hiding this comment.
We should just check where getItemTitle is called and not call it when it's a symbol (I'm assuming it's an internal call to data form component)
That would work to, its being called within the getValue function of the title field definition.
| @@ -15,6 +15,7 @@ const authorField: Field< BasePostWithEmbeddedAuthor > = { | |||
| id: 'author', | |||
| type: 'integer', | |||
| elements: [], | |||
| supportsBulk: true, | |||
There was a problem hiding this comment.
Should a type also be able to decide if it supports bulk or not?
Maybe only if no custom Edit is being passed in?
|
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. |
|
Size Change: +420 B (+0.02%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
@gigitux, @youknowriad, @oandregal this PR is ready for review, it is an initial stab at the bulk editing. I left some open questions and it will likely require some follow up, depending on how integral we want to make it. |
|
Flaky tests detected in ac03ed1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12122182106
|
|
Thanks for working on this! I'm considering whether For example, the slug field doesn't support bulk editing. An extension might need to have the same slug for different entities, given that the |
packages/dataviews/src/types.ts
Outdated
| /** | ||
| * Whether the action can be used as a bulk action. | ||
| */ | ||
| supportsBulk?: boolean; |
There was a problem hiding this comment.
Should we be more explicit? supportsBulkEditing or something like that. We may have bulk actions or something else.
There was a problem hiding this comment.
I like the supportsBulkEditing, done in 4df0165
| @@ -111,6 +113,10 @@ function PanelDropdown< Item >( { | |||
| [ popoverAnchor ] | |||
| ); | |||
|
|
|||
| const fieldValue = fieldDefinition.getValue( { item: data } ); | |||
| const showMixedValue = | |||
| isBulkEditing && ( fieldValue === undefined || fieldValue === '' ); | |||
There was a problem hiding this comment.
yeah value undefined or '' might be valid values, this is why I suggested the "Symbol"
| } ); | ||
| if ( intersects ) { | ||
| intersectingValues[ key ] = firstRecord[ key ]; | ||
| } |
There was a problem hiding this comment.
This is an exact replicate of this comment: #67344 (comment) I am not sure if you meant to do that or if GH messed up?
There was a problem hiding this comment.
Yes, forget about this one :P
There was a problem hiding this comment.
This is actually where I had my initial "Symbol" comment :)
There was a problem hiding this comment.
Aaaha that makes a lot of sense! :)
youknowriad
added
[Type] Enhancement
louwie17
force-pushed
the
add/65685_bulk_editing_support
branch
from
7fb76cf to
ac03ed1
Compare
| } ); | ||
| if ( intersects ) { | ||
| intersectingValues[ key ] = firstRecord[ key ]; | ||
| } |
There was a problem hiding this comment.
This is an exact replicate of this comment: #67344 (comment) I am not sure if you meant to do that or if GH messed up?
| id | ||
| ) | ||
| ) | ||
| : null, |
There was a problem hiding this comment.
I do seem to get this warning when adding the above, which I am not entirely sure why.
The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders.
There was a problem hiding this comment.
Or is there a better way to get the edited record for multiple entities?
| intersectingValues[ key ] = firstRecord[ key ]; | ||
|
|
||
| if ( typeof firstRecord[ key ] === 'object' ) { | ||
| // Traverse through nested objects. |
There was a problem hiding this comment.
Why do you think we should traverse nested objects?
There was a problem hiding this comment.
Some fields may rely on the nested values only, incase we want to support that for bulk editing. Although we don't have direct use case for that in core right now, aside from maybe metadata fields.
But if we go with your suggestion below, this won't be necessary as we can rely on the passed in fields only.
|
|
||
| /** | ||
| * Loops through the list of data items and returns an object with the intersecting ( same ) key and values. | ||
| * Skips keys that start with an underscore. | ||
| * | ||
| * @param data list of items. | ||
| */ | ||
| function getIntersectingValues< Item extends object >( data: Item[] ): Item { |
There was a problem hiding this comment.
I just noticed that this function is trying to combine the items into an Item object. Unfortunately, I don't think that's possible really because the Symbol is not a valid Item value and the nesting is not the right thing to do.
I think what we should be doing here is calling field.getValue for all the fields (maybe the fields that are within the form) and compute the result in a merged object (that is not Item) who keys are the field keys and values are the result of getValue calls.
This is necessary because we can't really assume/know the shape of valid Item. this is something we discussed IRL with @oandregal and @gigitux at Rome core days.
There was a problem hiding this comment.
Hmm yeah, I can give this approach a try. I agree it makes more sense, but will need quite a bit more changes I believe.
For example, if we do generate this new merged object using getValue and receive an array of data do we pass the array of data down to each field? or the merged object? or just the first item in the array ( probably the whole array, but currently a lot of fields pass that straight into getValue ).
There was a problem hiding this comment.
I agree it's not very straightforward. Maybe the current PR can be a good interim solution (although not entirely "correct"). Should we try it in a separate PR to see how they compare?
There was a problem hiding this comment.
Should we try it in a separate PR to see how they compare?
Yeah a different PR is a good idea, otherwise it may be hard to see the pros/cons of each approach.
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?
DataForm allows an array of items to be passed in.
When an array of items is passed in it does a couple things:
bulkEditingand only shows fields withbulkEditingsupport.Some questions/follow up:
getValue, but this will cascade into quite a few other changes.getValueand pass thedataobject by default, this will break if thedataobject can also be an array.onChangemethod with the intersecting changes only make sense? or are we better off passing the array back with the updates.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
bulk-editing.mp4