-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Block fields: Add object type support for media and link controls #74435
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
base: add/field-type-object
Are you sure you want to change the base?
Block fields: Add object type support for media and link controls #74435
Conversation
|
Size Change: +65 B (0%) Total Size: 3.08 MB
ℹ️ View Unchanged
|
| getValue: ( { item } ) => { | ||
| if ( fieldDef.mapping ) { | ||
| // Extract mapped properties from the block attributes | ||
| // When a field is an object, flatten all the properties to the root | ||
| // of the block attributes. | ||
| if ( fieldDef.type === 'object' && fieldDef.properties ) { | ||
| const mappedValue = {}; | ||
| Object.entries( fieldDef.mapping ).forEach( | ||
| ( [ key, attrKey ] ) => { | ||
| mappedValue[ key ] = item[ attrKey ]; | ||
| } | ||
| ); | ||
|
|
||
| // Normalize to canonical structure based on field type | ||
| if ( fieldDef.type === 'media' ) { | ||
| return normalizeMediaValue( mappedValue, fieldDef ); | ||
| } | ||
| if ( fieldDef.type === 'link' ) { | ||
| return normalizeLinkValue( mappedValue, fieldDef ); | ||
| } | ||
|
|
||
| // For other types, return as-is | ||
| // Convert to field keys. | ||
| Object.keys( fieldDef.properties ).forEach( ( key ) => { | ||
| const attributeKey = | ||
| fieldDef.properties[ key ].id ?? key; | ||
| if ( item[ attributeKey ] ) { | ||
| mappedValue[ key ] = item[ attributeKey ]; | ||
| } | ||
| } ); | ||
| return mappedValue; | ||
| } | ||
| // For simple id-based fields, use the id as the attribute key | ||
| return item[ fieldDef.id ]; | ||
| }, | ||
| setValue: ( { item, value } ) => { | ||
| if ( fieldDef.mapping ) { | ||
| // Denormalize from canonical structure back to mapped keys | ||
| let denormalizedValue = value; | ||
| if ( fieldDef.type === 'media' ) { | ||
| denormalizedValue = denormalizeMediaValue( | ||
| value, | ||
| fieldDef | ||
| ); | ||
| } else if ( fieldDef.type === 'link' ) { | ||
| denormalizedValue = denormalizeLinkValue( | ||
| value, | ||
| fieldDef | ||
| ); | ||
| } | ||
| setValue: ( { value } ) => { | ||
| // When a field is an object, flatten all the properties to the root | ||
| // of the block attributes. | ||
| if ( fieldDef.type === 'object' && fieldDef.properties ) { | ||
| const mappedValue = {}; | ||
|
|
||
| // Build an object with all mapped attributes | ||
| const updates = {}; | ||
| Object.entries( fieldDef.mapping ).forEach( | ||
| ( [ key, attrKey ] ) => { | ||
| // If key is explicitly in value, use it (even if undefined to allow clearing) | ||
| // Otherwise, preserve the old value | ||
| if ( key in denormalizedValue ) { | ||
| updates[ attrKey ] = | ||
| denormalizedValue[ key ]; | ||
| } else { | ||
| updates[ attrKey ] = item[ attrKey ]; | ||
| } | ||
| // Convert to attribute keys. | ||
| Object.keys( fieldDef.properties ).forEach( ( key ) => { | ||
| if ( value[ key ] ) { | ||
| const attributeKey = | ||
| fieldDef.properties[ key ].id ?? key; | ||
| mappedValue[ attributeKey ] = value[ key ]; | ||
| } | ||
| ); | ||
| return updates; | ||
| } ); | ||
| return mappedValue; | ||
| } | ||
| // For simple id-based fields, use the id as the attribute key | ||
| return { [ fieldDef.id ]: value }; | ||
| }, |
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.
getValue and setValue is I think where the main code of interest is.
The thing that was unexpected to me is that I still had to map back and forth from the field keys to the attribute keys. I assumed the dataform would do this for me. Maybe it's something I missed, or perhaps it's because a custom getValues / setValues is being declared.
The other thing is that although we declare the field as having a structure like this:
{ image: { id, url, caption } }
The block attributes are all a flat object:
{ id, url, caption }
So in getValue/setValue the properties need to be flattened/unnested. This could be another reason why the mapping between keys is still required 🤔
122b8ab to
3742e56
Compare
| } | ||
| // Convert to attribute keys. | ||
| Object.keys( fieldDef.properties ).forEach( ( key ) => { | ||
| if ( value[ key ] ) { |
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'm still getting my head around this and #74409 but just noticed this line... will value[ key ] always be truthy? I know the PR is still in draft, so didn't want to jump ahead.
| if ( key === 'caption' || key === 'alt' ) { | ||
| resetValue[ key ] = ''; | ||
| } | ||
| resetValue[ key ] = undefined; |
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.
will this overwrite an empty string?
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.
This is already looking like a good improvement. Judging from @oandregal's comments / latest update in #74409 it looks like we can try out the group field type. From my reading of it, although it doesn't include the custom getValue and setValue methods, it seems we'd still need to handle mapping? I'm wondering if it's okay for the consumer to do it, or if there's additional logic that should happen on the field's end (in the DataViews package).
Apologies if I'm missing something obvious, though!
… to demonstrate changes
6faf7f1 to
cfb0fba
Compare
What?
Based on #74409
Takes some steps towards aligning the block fields implementation with dataforms/fields APIs.
#74409 proposes adding an
'object''group' type that has sub properties, which would be useful for media and link fields. This PR adopts it.How?
The PR generally switches away from using a custom
typefor the field types instead adding anEditproperty to implement the custom controls for fields.The
mappingproperty becomesproperties.In general I've tried to remove as much special code as possible from the implementation to make it closer to a native DataForm, but some mapping from field properties to attributes still seems to be required. I'm not sure if this is due to something wrong in the implementation of block fields.
Testing Instructions
Prerequisite: Enable the two 'contentOnly' gutenberg experiments