DataForm - Add combined fields support

[louwie17]

What?

This PR adds support for combinedFields layout support to the DataForm panel view. This follows a similar configuration as combinedFields in DataViews.

cc: @youknowriad, @oandregal

Why?

This is needed to allow fields multiple fields to be rendered as part of a single row, similar to how currently the the post status field works within the site editor. This will be needed for the DataForms as well.

Part of #64519 and an initial step to implementing the Password field.

How?

This adds a layout option to the form config that accepts a combinedFields array. A combined field can contain children and direction, it also accepts a custom render function to display the data. This follows the same format as introduced here for DataViews.

Open questions:

• In the design the field label is different from the top label within the modal, do we provide a separate config for this?
• How would this look in the regular view?

An alternative to consider, is not to alter this through the layout, but provide a layout type as part of the field type definition that supports a direction and children.

{
  type: 'layout',
  children: [ 'title', 'status' ],
  direction: 'vertical'
}

Testing Instructions

1. Run storybook: npm run storybook:dev
2. Go to the DataViews > DataForm > Combined Fields story
3. See the combined field label Status & Visibility and the two fields underneath ( status & password )
4. Now change the form type to panel
5. Select the Status & Visibility field, it should render a status and password field.
6. Edits should be correctly reflected within status & password fields across the form types.

Testing Instructions for Keyboard

Screenshots or screencast

showcase-combined-fields.mp4

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: louwie17 <louwie17@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: gigitux <gigitux@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[youknowriad]

What is this function about?

The thing that I'm wondering personally is whether for "forms" these are combined fields or more things like "groups"...

[youknowriad]

I think having more example of how this translates in actual forms and panels would help make the right abstraction.

[louwie17]

What is this function about?

Its suppose to be the same as the render function of the field config, but its acting more like the getValue function.

The thing that I'm wondering personally is whether for "forms" these are combined fields or more things like "groups"...
I think having more example of how this translates in actual forms and panels would help make the right abstraction.

Groups would probably fit this use case better, the main example is the status field that includes the status radio control, posts schedule, the password field and also the sticky checkbox:

gutenberg/packages/editor/src/components/post-status/index.js

Line 197 in 2943dd3

<form>

password-conditional.mp4

We could also create a single status field that includes all of this logic, we did have to remove the radio dependency here:

gutenberg/packages/edit-site/src/components/post-fields/index.js

Lines 283 to 294 in 3ec1ced

	{
	label: __( 'Status' ),
	id: 'status',
	type: 'text',
	elements: STATUSES,
	render: PostStatusField,
	Edit: 'radio',
	enableSorting: false,
	filterBy: {
	operators: [ OPERATOR_IS_ANY ],
	},
	},

This is mostly a use case for the panel view. For the regular form one could render multiple regular data forms to create the grouping. Although there would be some additional use cases like the column use case for displaying two fields beside each other ( we have a similar use case in the product form for regular and sale price ):

[louwie17]

The thing that I'm wondering personally is whether for "forms" these are combined fields or more things like "groups"...

Would you envision "groups" to be like field types that accept children, or act in a similar manner as the combinedField API?

[youknowriad]

Its suppose to be the same as the render function of the field config, but its acting more like the getValue function.

Do you think we can guess this from the "fields" or is the "render" function mandatory here? I'm also curious how the "combined fields" or "groups" feature translate in regular forms. What's the output there?

[louwie17]

Is this PR working for the regular form? I don't see any visual grouping there:

Yeah that would require extra styling, the only visible option in the regular form is the addition of the Status & Visibility label underneath the title. But there is no clear distinction of when that ends.

Does it make sense to render these as a fieldset being the combined field's label its legend?

Yeah fieldset makes sense I will update that.

[louwie17]

How would those work with the combinedFields API introduced here?

Good questions 👍

being able to stack fields horizontally or vertically, even if they are not "combined fields"

Currently I followed the DataViews and support the direction prop that will render it horizontally or vertically. Currently its hard to do if they are not "combined fields" we did have to merge them into a single field or nest DataForms.
It may be worth pulling this out of combined fields and have a specific horizontal and vertical layout, similar to what JSON Forms does: https://jsonforms.io/examples/layouts/#horizontal-layout ?

One question I did have is for the direction, does that always look the same in the regular form and the panel form, or may a form overwrite this? This may be a "moo" point though as storybook may be the only place where the form type gets toggled on a constant basis.

being able to have to provide a different field combination depending on a field value:

I currently did that by updating the form object directly and updating the children if in this case the status criteria changes: https://github.com/louwie17/gutenberg/pull/1/files#diff-4ed486e973856f952d2b80aea381fc1f5a1d0004995a0b68554147b02cee0b55R76-R87

Now this isn't so bad when talking about a very small form as it is now, but I don't like this idea if we have a lot more conditional logic that depends on different data ( meaning the form object gets regenerated a lot ).

I was kind of thinking that just like the isValid function we may want a isVisible function, that in the future will also support the JSON Schema validation rules in support of the rule parser that Nadir is working on. This way visibility can be handled on a per field level and avoids re-rendering of the entire form.

[oandregal]

One question I did have is for the direction, does that always look the same in the regular form and the panel form, or may a form overwrite this?

My approach is to think about the API we want to introduce at a conceptual level. In this case, we're trying to introduce the idea of "grouping fields". Does that feature make sense for both layouts? It think it does, even if it means different things for different layouts at a visual level. There may be cases where an API/feature only makes sense for a given layout. It'd be ok to ignore the other layout in that case. Does this help?

I was kind of thinking that just like the isValid function we may want a isVisible function

I like this direction better than the current one.

Perhaps we can start from this angle? This could be a separate PR to implement the visibility feature. This PR's scope could be just about clarifying the API to group fields. (both things can land & be worked in parallel).

[youknowriad]

Agree about decoupling visibility/conditional rendering and grouping.

I will add that ultimately, I see different level or ways to group fields: tabs, vertical or horizontal fieldsets (kind of like here) and maybe other ways we're not seeing right now. Just food for thought to come up with the right approach.

Something to consider as well is that even the layout.type can be seen as "not necessary" and instead we could just imagine something like:

const fields = [ 
   { field: 'author', control: 'panel-row' }, 
   { field: 'title', control: 'inline' }, 
   { field: 'some group', control: 'group', fields: [ 'child1', 'child2' ] }

in other words, instead of having a layout type, you can just mix and match how to render the fields. In the same form you can have some inline controls, grouped or not and other fields that behave like the panel (dropdown)...

[louwie17]

in other words, instead of having a layout type, you can just mix and match how to render the fields. In the same form you can have some inline controls, grouped or not and other fields that behave like the panel (dropdown)...

Hmm yeah that could work, we did have to restructure our current panel and regular view to handle a single field instead of a list of fields and render each layout type respectively of the field.
With this principle the group structure makes a bit more sense, but I wonder if the use case of combining panel and regular/inline fields would be heavily used.
Makes me still think we could use layout.type as the default.

[gigitux]

Thanks for working on this! Overall, it looks good. I left a few minor comments!

[gigitux]

It is a minor, but I noticed that the panel to customize props doesn't appear on the new story:

[louwie17]

You only notice that on the Docs page correct? I think that is normal, at-least I see the same in other stories.
If you select the combined fields specific story from the navigation on the left the props should show up:

[louwie17]

@gigitux thanks for the review, I addressed your comments, it should be ready for a re-review :)

[youknowriad]

Two suggestions/questions:

• Any reason why the "status" label is hidden in the "panel" mode.
• I understand the reasoning for the "black border" around the combined fields in the "form" layout but I feel like it's kind of poorly designed. @jameskoster Do we have any idea about how we want "field combination" to work in regular forms?

[jameskoster]

The padding on those popovers looks too small, I guess that's a separate issue.

Do we have any idea about how we want "field combination" to work in regular forms?

In #63624 we have the idea of arranging certain inputs into a two-column layout. Do you think it would make sense to use this combining feature to govern the layout?

Otherwise I don't know that combined inputs necessarily need any additional styling. That said it might be good to wrap them in a class or fieldset so that we can style later.

[louwie17]

• Any reason why the "status" label is hidden in the "panel" mode.

Good call out, I mimicked this from the current Status panel within the site editor. Which uses the main label as the top field label ( see picture ).
But I am not entirely sure if we want to make this the default and instead make use of hideLabelFromVision to control this on a per field level. Or show the field specific labels always?
@jameskoster any thoughts around this? if the item in question makes sense around showing all the labels. There is a bit of inconsistency as you can see with the picture below where

• We have the main label Status & visibility and hide the label of the status field
• We show a Publish label and also show the time & date labels for the respective fields
• We don't show a specific label for the password protected, but just the checkbox directly. With showing the label of the password text field.
  • We do the same for the Sticky checkbox in posts.

• I understand the reasoning for the "black border" around the combined fields in the "form" layout but I feel like it's kind of poorly designed. @jameskoster Do we have any idea about how we want "field combination" to work in regular forms?

Yeah changing the black border is fine, currently that is auto set by the browser as it is a fieldset now. I just overwrote the CSS for it in the panel view.

[youknowriad]

Good call out, I mimicked this from the current Status panel within the site editor. Which uses the main label as the top field label ( see picture ).
But I am not entirely sure if we want to make this the default and instead make use of hideLabelFromVision to control this on a per field level. Or show the field specific labels always?

I would vote for consistency now and adding the label for all the subfields.

[youknowriad]

Do we really need the "border" between the fields in the panel layout? I feel like it's not necessary there.

I also think the spacing (padding for the field set and the padding within the drop-downs) is not great but I think this is not specific to this PR. We can probably tidy these things up separately.

[youknowriad]

Personally, I feel like the best approach is going to be to remove the "layout" type entirely and move the "combined fields" definition within the fields array.

fields: [ 'title', { some combined field definition } ]

That said, I'm ok if we land this and consider that change separately.

[louwie17]

Do we really need the "border" between the fields in the panel layout? I feel like it's not necessary there.

Yeah I can remove that, this is also something the can be left up to the consumer of the package.

I also think the spacing (padding for the field set and the padding within the drop-downs) is not great but I think this is not specific to this PR. We can probably tidy these things up separately.

Agreed! Lets do this as a follow up.

Personally, I feel like the best approach is going to be to remove the "layout" type entirely and move the "combined fields" definition within the fields array.
fields: [ 'title', { some combined field definition } ]
That said, I'm ok if we land this and consider that change separately.

Yeah I can explore that as a follow up. Initially I wasn't sure if we were hardset on the string array for the form.fields definition. I think having the array support a combined field definition does simplify things.

[louwie17]

I just rebased and removed the border styling.
@youknowriad I also don't have merge permissions in GB yet, so I would have to get you or André to merge it.