ContentOnlyControls: Refactor to use DataForm #73374
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Code Quality
andrewserong
force-pushed
the
try/switch-content-only-controls-to-dataform
branch
from
12ff5ec to
c13fd67
Compare
|
Size Change: +21.7 kB (+0.86%) Total Size: 2.54 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 61164f9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19489722408
|
6 tasks
6 tasks
andrewserong
commented
Nov 19, 2025
Comment on lines
+41
to
+51
| /** | ||
| * Creates a configured control component that wraps a custom control | ||
| * and passes configuration as props. | ||
| * | ||
| * @param {Object} config - The control configuration | ||
| * @param {string} config.control - The control type (key in CONTROLS map) | ||
| * @return {Function} A wrapped control component | ||
| */ | ||
| function createConfiguredControl( config ) { | ||
| const { control, ...controlConfig } = config; | ||
| const ControlComponent = CONTROLS[ control ]; |
Contributor
Author
There was a problem hiding this comment.
This function is effectively a copy of the same one that exists internally within DataForm. The goal here is to try to get these link, media, and richtext components a bit closer to how they might look if they were bundled controls within DataForm. We're not there yet with this PR — the intention here is to just take a step in that direction and to continue to refine in follow-ups.
This function is so that we can bundle configuration alongside using controls like media, link, or richtext.
andrewserong
commented
Nov 19, 2025
| id: fieldDef.id, | ||
| label: fieldDef.label, | ||
| type: fieldDef.type, // Use the field's type; DataForm will use built-in or custom Edit | ||
| config: { ...fieldDef.args, defaultValues }, |
Contributor
Author
There was a problem hiding this comment.
Not for now, but potentially in a follow-up we might move these from fieldDef.args to be properties of fieldDef.Edit. Similarly the config in this object might move to Edit config, too. I haven't gone further in this PR because I wanted to kind of mark a point of iteration and see if this is good enough for now.
andrewserong
requested review from
ajitbohra,
ellatrix and
fabiankaegy
as code owners
|
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. |
talldan
reviewed
Nov 19, 2025
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
This is testing well, so TBH, I'm kinda happy to approve it. The only thing I notice from a UX point of view is that this dropdown isn't to the side of the inspector like other dropdowns:
Most of the comments are questions or observations rather than requests to change anything.
Comment on lines
-58
to
-60
| mapping: { | ||
| value: 'caption', | ||
| }, |
Contributor
There was a problem hiding this comment.
It's cool to see these removed in favour of using the id 👏
Comment on lines
114
to
118
| href: value?.href ?? value?.url ?? '', | ||
| url: value?.url ?? value?.href ?? '', // url falls back to href | ||
| rel: value?.rel ?? '', | ||
| target: value?.target ?? value?.linkTarget ?? '', | ||
| linkTarget: value?.linkTarget ?? value?.target ?? '', |
Contributor
There was a problem hiding this comment.
I'm curious why both href/url are needed, also target/linkTarget, as they seem to normalize to the same values. The attributes are also passed through the mapping process which should remove the inconsistencies.
Maybe some of the reasoning can be documented in inline comments.
Contributor
Author
There was a problem hiding this comment.
Oh, this was a double-up while I was debugging and I forgot to remove it! Thanks for flagging, I've pushed an update to standardize, hopefully I haven't broken anything in the process 😄
| } | ||
|
|
||
| const result = {}; | ||
| Object.entries( fieldDef.mapping ).forEach( ( [ key ] ) => { |
Contributor
There was a problem hiding this comment.
Not a blocker, as I realize this is still going to pass through several revisions, but it looks like normalize/denormalize aren't complete opposites as this one uses the mapping while normalize doesn't.
It feels like a gotcha because when functions are named like this I'd usually expect them to losslessly reverse the operation of the other.
Contributor
Author
There was a problem hiding this comment.
Yes, I've pushed an update that makes these a bit closer to each other, so normalize uses the mapping
Comment on lines
+216
to
+218
| if ( fieldDef.type === 'media' ) { | ||
| return normalizeMediaValue( mappedValue, fieldDef ); | ||
| } |
Contributor
There was a problem hiding this comment.
I guess in future we might want this to become part of the field definition (in the media folder), and make it so that an if statement (which isn't really scalable to other field types) isn't required.
| } | ||
|
|
||
| // Merge with existing value to preserve other field properties | ||
| updateAttributes( { ...value, ...resetValue } ); |
Contributor
There was a problem hiding this comment.
Would the plan be to make these use field.setValue in the future?
Contributor
Author
There was a problem hiding this comment.
Yes! While troubleshooting, I found it easier for this PR to just pop updateAttributes in here for now, but I'd like to refactor this in a follow-up, too
| const { allowedTypes = [], multiple = false } = config; | ||
|
|
||
| if ( multiple ) { | ||
| return 'todo multiple'; |
Contributor
There was a problem hiding this comment.
I guess we should get rid of this at some point 😆
Contributor
Author
|
Thanks for the feedback, @talldan! I've pushed a few updates to hopefully streamline things a little bit and settle on a common name for some of the keys. I've switched to 
|
talldan
approved these changes
Nov 20, 2025
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
Thanks for the changes, this is working well, very similar to before, so looks good to move forward with this refactor to DataForm. Nice work!
Contributor
Author
andrewserong
deleted the
try/switch-content-only-controls-to-dataform
branch
11 tasks
This was referenced Nov 20, 2025
oandregal
reviewed
Nov 21, 2025
| label: __( 'Audio' ), | ||
| type: 'Media', | ||
| type: 'media', | ||
| shownByDefault: true, |
Member
There was a problem hiding this comment.
I've prepared #73479 so the block type declares both the fields and the form for DataForm. We were already doing that, but indirectly via shownByDefault prop, so 73479 makes it obvious.
Member
|
It's great to see usage of Field API and DataForm in more places 👏 I'd like to offer a suggestion: the blocks should only use what's available in the Field API. Being strict about this simple constraint will help us collaborate better and go faster. In this phase of the project, we need to uncover any missing pieces of the Field API as fast as possible. The current code that is shipped is a totally different API and doesn't help with that goal. If we keep the current direction, I'm concerned that it'd be difficult to reconcile the two APIs later — and it'd be harder to ship this great work in the 7.0 cycle. In practical terms, this means:
{
id: 'link',
type: 'link',
shownByDefault: true,
mapping: {
url: 'url',
rel: 'rel',
linkTarget: 'linkTarget',
},
}becomes: {
id: 'link',
type: 'object',
properties: {
url: 'url',
rel: 'rel',
linkTarget: 'linkTarget',
},
Edit: 'link', // consider why the existing 'url' is enough? should we expand it?
}
{
id: 'audio',
label: __( 'Audio' ),
type: 'media',
shownByDefault: true,
mapping: {
id: 'id',
url: 'src',
},
args: {
allowedTypes: [ 'audio' ],
multiple: false,
},
},becomes: {
id: 'audio',
label: __( 'Audio' ),
type: 'object',
props: {
id: 'id',
url: 'src',
},
Edit: {
control: 'media',
allowedTypes: [ 'audio' ],
multiple: false,
},
},Take the examples I shared as illustration of the deviations we've taken from the Field API, and not as something normative. It's been only a few hours since I've looked at this code, and I'm sure I'm missing context. The essential part is about being strict with this constraint: the blocks should only use what's available in the Field API at any given point in time. Any other thing is a detail we need to figure it out together, as we make progress. I'm happy to pair with any of you next week to migrate the existing config to using the Field API. I feel it's important that we align as soon as possible, otherwise the drift will increase, and it'll harder to reconcile later. |
Contributor
Author
Thanks for flagging @oandregal — to be clear, it was always our intention to move in that direction! This PR was intended as a single step in that direction, with the code that's currently messy safely tucked away behind an experiment. @talldan was especially keen to make sure we don't expose any of the currently messy state, and so the I'm occupied with other tasks this week, but also happy to help with iterative PRs that progress us toward using the DataForm and Fields APIs correctly. If you or @talldan make a start before I'm able to jump in properly, I'm happy to help review in the interim! |
Contributor
Author
|
I've updated the To-do list in the linked issue (#73261) to capture @oandregal's excellent list of follow-up tasks above. Thanks again! |
Contributor
|
@oandregal for the "type" object, I think we should consider nesting fields. |
This was referenced Nov 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #73261
Follows: #71730
Refactor the ContentOnlyControls to use DataForm for rendering editable fields. Note that this PR is an iterative step in this direction, it doesn't get the whole way there, but it (hopefully) sets things up to continue to make improvements in follow-ups. The goal is to not break things while we gradually improve how we're structuring things.
Why?
The editable fields in the right-hand sidebar for ContentOnlyControls should ideally use familiar components and interactions used elsewhere in Gutenberg (namely DataForm), so this PR is a step in that direction. Note that we don't (yet) have generalised media, link, or richtext fields, so those continue to be ad-hoc. However for simple text fields, like alt text on the Image block, we can re-use the
textcontrol from DataForm.The idea with this PR is that we could eventually look at moving the ad hoc media, link, and richtext fields over to DataForm once we can build common components for them.
How?
settings.fieldsstructure slightly to useidfields andtypes that more closely match DataForm — it's not an exact match, but it gets things a little closerA caveat: the mapping isn't very neat right now, and things are pretty messy in regards to how we're passing values into the control components. If it looks acceptable, my plan would be to try to merge this PR and then iteratively work on each of those controls.
Testing Instructions
Screenshots or screencast
This should show a UI that's very close to what landed in #71730 with panel headers that look like the ToolsPanel. Here's a quick demo:
2025-11-19.15.05.50-optimised.mp4