Move DataForm layout to the field level #66531
Conversation
|
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. |
louwie17
added
[Type] Enhancement
| @@ -0,0 +1,41 @@ | |||
| /** | |||
There was a problem hiding this comment.
Added a DataForm context to store the field definitions. This way I can more easily manage the layout on a field level without having to make sure I drill down the field definitions.
|
Thanks for keep using the storybook as an example, that's really valuable! 👏 What do you think of doing the reverse example? In the combinedFields story ( There's some other challenges with this approach like making the styles work well together (note how the title "Status" has different styles/font size) that make the current example look a bit weird: 
|
packages/dataviews/src/components/dataform/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
packages/dataviews/src/types.ts
Outdated
| @@ -536,12 +536,21 @@ export type NormalizedCombinedFormField< Item > = CombinedFormField< Item > & { | |||
| Edit?: ComponentType< DataFormCombinedEditProps< Item > >; | |||
| }; | |||
|
|
|||
| export type FormField = | |||
There was a problem hiding this comment.
Based on what I saw so far, this is the API I was expecting for FormField:
export type FormField =
| string // The field id
| {
id: string; // The field id
layout?: '...';
fields?: FormField[]; // Allows grouping fields
};Some thoughts:
- Why do we need
fieldinFormField? - Can the
form.combinedFieldsAPI be removed? Why do we need both FormField and combinedFields?
There was a problem hiding this comment.
I reckon some of this is pre-existing to this PR. If you think it's too much for this PR, we could do a PR to simplify the implementation first, and then rebase this PR to use that.
There was a problem hiding this comment.
Why do we need field in FormField?
Actually my thought was to use field for the field id instead of id, as field is more clear. We did probably want to replace fields with children to not make it to confusing.
Mostly for this scenario, where what would you do if you wanted to display a horizontal column layout within a panel. Or any horizontal layout in general. In this case we are just grouping fields, without needing to reference a specific field.
It may look like this:
{
id: "status",
layout: "panel",
fields: [
{
layout: 'horizontal' // if we go this route
fields: [ 'status', 'password' ]
}
]
}
We may still want to add an id here as a key, although we could also generate one.
Can the form.combinedFields API be removed? Why do we need both FormField and combinedFields?
Yeah combinedFields can be removed, as this API replaces that functionality. I was thinking of doing that as a follow up. But happy to remove it as part of this PR.
There was a problem hiding this comment.
We did probably want to replace fields with children to not make it to confusing.
Agree to this, it'd make the code clearer.
Why the regular layout can't take children but the panel layout can? The existing combinedFields API worked for both.
There was a problem hiding this comment.
Another thought is: why only the fields that are defined via object in form.fields should have the layout property? It looks like this is optimizing for the current use case (combining status + password), but I wonder if the layout should be part of the Fields API instead?
|
|
||
| export default function DataForm< Item >( { | ||
| form, | ||
| ...props | ||
| }: DataFormProps< Item > ) { | ||
| const layout = getFormLayout( form.type ?? 'regular' ); | ||
| if ( ! layout ) { | ||
| const normalizedFields = useMemo( |
There was a problem hiding this comment.
I'm mostly interested in getting the API right in the review. However, I've also noticed something related to implementation that we could perhaps simplify.
Can we do the following here (pseudo-code):
// Fields are only the visible fields
// and contain all the information they need including the layout.
const fields = normalizeFields( fields, form );By making sure the normalized fields have all the info they need and it's done as the first step:
- we don't need the context, because the fields are already passed down as they are needed via props, like the
form.fieldswork now - we can remove a lot of code that normalizes in the leaf components. Examples: formregular, formpanel, dataformlayout, etc.
There was a problem hiding this comment.
If I understand you correctly, you mean replacing the form.fields array with the normalized fields already. So [ "status" ] would turn into [ { ...status field definition } ]?
I am ok with this approach, but initially avoided it because of the nesting possibility. This means we will have to traverse through the entire field tree initially already, instead of having the react components deal with this naturally.
we can remove a lot of code that normalizes in the leaf components. Examples: formregular, formpanel, dataformlayout, etc.
With this new implementation FormRegular and FormPanel can actually be removed as they are replaced by FormRegularField and FormPanelField.
DataFormLayout would be the only exception.
There was a problem hiding this comment.
As it is, normalization is spread across multiple layers/components: normalizeField utility, the dataform context (for the label), the dataform layout (for the field layout).
What do you think of 1) making layout part of the Fields API, 2) adding support for children in both regular&panel, 3) centralizing everything in normalizeFields (removing the context)?
There was a problem hiding this comment.
@oandregal it's important to distinguish form.fields from the fields prop which represents the "schema" of our data basically.
The function being called here, normalized the "schema" and doesn't touch form.fields at all. We could obviously introduce normalizeForm function or something (which is I believe what you're suggesting) but it's important to keep these things distinct. normalizeFields is a function that is common between DataViews and DataForms.
There was a problem hiding this comment.
I agree normalizeFields is common across DataViews/DataForm so it shouldn't have the form argument and we may need another function or just have some code here. That's fine. My point was about simplifying the implementation. Let me try to rephrase a bit to see if it comes across:
The way I see it, form.fields is a filter on fields (it's also creating new ones, for example, combined fields). So, if we normalize the fields as early as possible, all internal components can just work with the Fields API (fields).
As I mentioned elsewhere, this conversation is not a blocker to for me. I'm mostly interested in getting the API right.
There was a problem hiding this comment.
The way I see it, form.fields is a filter on fields (it's also creating new ones, for example, combined fields). So, if we normalize the fields as early as possible, all internal components can just work with the Fields API (fields).
That makes sense, my only thought would be if there could be a case where "a field definition is needed for a field that is not in the rendered list ( form.fields )?"
Like:
- A conditional field
- Should not be needed because of the
isVisiblefunction, and updatingform.fieldson the fly wouldn't be necessary anymore.
- Should not be needed because of the
- A field that requires data of another field definition and may need to use
getValueinternally ( the slug comes to mind, as it would be auto generated from the title ).- This would be avoided if we still expose the
fieldsdefinitions anyway.
- This would be avoided if we still expose the
louwie17
force-pushed
the
update/dataform_combined_fields_api
branch
from
a707275
to
58150ef
Compare
|
Size Change: +423 B (+0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
| /> | ||
| ); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Added sticky example, which flows nicer for the inline example.
| typeof field !== 'string' && field.label | ||
| ? field.label | ||
| : definition.label, | ||
| }; |
There was a problem hiding this comment.
I added the label option to the FormField definition, which overwrites the field definition label if provided.
I did this for two reasons:
- This way the individual layouts won't need to worry about this
- The regular layout doesn't handle the label rendering and renders the Edit component directly, which would require us to overwrite the label there anyway.
| if ( | ||
| ! fieldDefinition || | ||
| ( fieldDefinition.isVisible && | ||
| ! fieldDefinition.isVisible( data ) ) |
There was a problem hiding this comment.
Added the isVisible logic here
| visibleFields.push( | ||
| ...normalizeCombinedFields( combinedFields, fields ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Removed the combined fields related stuff
|
Thanks for the initial review @oandregal, I did some clean up and addressed some of your suggestions. I also left some replies to your above comments.
Good suggestion on the reverse, I end up creating a simple |
|
Thanks for updating! It looks like the initial status for the layout of the Screen.Recording.2024-10-31.at.17.11.13.mov |
|
Flaky tests detected in ddf996a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11684836851
|
|
This is nice, thanks for the effort! The main blocker I see is adding support for |
louwie17
force-pushed
the
update/dataform_combined_fields_api
branch
from
5e91b52
to
ddf996a
Compare
| @@ -57,16 +58,31 @@ function DropdownHeader( { | |||
| ); | |||
| } | |||
|
|
|||
| function FormField< Item >( { | |||
| function PanelDropdown< Item >( { | |||
There was a problem hiding this comment.
Moved the Dropdown code to its own component to better handle the labelPosition in the FormPanelField component.
packages/dataviews/src/types.ts
Outdated
| label?: string; | ||
| layout?: 'regular' | 'panel'; | ||
| labelPosition?: 'side' | 'top'; | ||
| children?: Array< FormField | string >; |
There was a problem hiding this comment.
It's weird to me that "children" is part of FormField. In other words, this would make more sense to me:
type SimpleFormField = {
id: string; // could be named "field" as well if we want to match "filters"
layout: 'regular' | 'panel';
labelPosition: 'side' | 'top';
}
type CombinedFormField = {
id: 'string'; // actually not sure this is necessary as it's just a random string but maybe it can be useful
label: 'string';
layout: 'regular' | 'panel';
children: Array<FormField>
}
type FormField = string | SimpleFormField | CombinedFormField;It is possible that I'm misunderstanding combined fields, but for me, it's weird to make a field children of another field
There was a problem hiding this comment.
I see the example of "status" with "password". Is "password" a child of "status" or we're creating a "virtual combined field" that combines both in a single panel. I feel the latter is closer to reality.
I guess the main difference is that I'm forcing the "label" and "children" to be required for "combined fields" and I'm removing "label" for regular fields because the label is inherited from the field directly and also preventing "regular" fields to have childre.
There was a problem hiding this comment.
I might be getting into details that don't matter much at the moment, so feel free to take this comment with a grain of salt (fine to ignore for now)
There was a problem hiding this comment.
No worries of digging into the details, this part of the API is what probably needs the most fine tuning anyhow.
It is possible that I'm misunderstanding combined fields, but for me, it's weird to make a field children of another field
True, not ideal, and I like your suggestion of separating it out again. I was running into this in the code as well, in particular with the panel layout as I was adding the field it-self to its children:
The panel layout is actually what I have been struggling with the most in regards to this, as it goes a bit against the grain compared to the regular layout.
For example in the regular form, it would make sense to wrap children up in combined fields, as all is required is a label and a wrapper in essence.
The panel requires the label, but also the render method, but in the status case we display a single value and render multiple edit fields ( already requiring some custom logic ).
So with using the above CombinedFormField and panel layout, we may still need a reference to a render method. In which case I defaulted to the field referenced as part of the id. But maybe the first one in the children list would suffice?
Sorry that may came across confusing.
I see the example of "status" with "password". Is "password" a child of "status" or we're creating a "virtual combined field" that combines both in a single panel. I feel the latter is closer to reality.
Its the latter, a "virtual combined field". You can see my slight frustration above with how this exactly fits within the current API structure.
There was a problem hiding this comment.
So with using the above CombinedFormField and panel layout, we may still need a reference to a render method. In which case I defaulted to the field referenced as part of the id. But maybe the first one in the children list would suffice?
Sorry that may came across confusing.
No it actually make sense. I'm fine with using the first field to start with (I like starting small), but it does highlight that maybe at some point a custom render function receiving the children could be defined (like concatenating the values or things like that) :)
There was a problem hiding this comment.
I added the above type as part of this commit: 21f7f9e
It works ok, at the moment it did require a few type checks in additional places. So there is room for cleanup, but that could be also be done as a follow up IMO.
Currently I have both layout fields ( regular & panel ) handle the combined fields. But I wonder if I can create a better abstraction for combined fields that would use the regular & panel under the hood. Again easier for the regular view instead of the panel view.
Also the only real difference for the panel view is that we use the first child for the render view. This is not a 100% clear from the code, and would require some clarification.
louwie17
force-pushed
the
update/dataform_combined_fields_api
branch
from
ddf996a
to
21f7f9e
Compare
|
Alright this should be ready for a re-review, I think it does require some additional clean up, but it does work with the new layout of the API if we are on board with that: https://github.com/WordPress/gutenberg/pull/66531/files#diff-05cd2939bbf237a5dbcd116a0da9c9019b05ec1ceaa9d429c5fc872f149f63c4R528-R542 If the API structure does seem good, than I would be more inclined to merge this if it in general looks good and address any clean up as follow up ( but happy to change my mind on this ). |
| ] as Field< SamplePost >[]; | ||
|
|
||
| export const Default = ( { type }: { type: 'panel' | 'regular' } ) => { | ||
| function toFormField( |
There was a problem hiding this comment.
I'm a bit confused as to why we need this? Isn't this part of what DataForm does internally? The other consumer (edit-site post-edit) doesn't require it (and it shouldn't).
There was a problem hiding this comment.
This is not necessary by consumers, only if they need to use a labelPosition that isn't the default.
I only added this in Storybook because it makes it easier to switch between the layout & labelPosition argument types, mostly for display purposes.
If this is confusing, what I could do is only use a single field as an example for the labelPosition versus the entire form.
|
This PR also fixes an issue I ran into while working with the status field at #66937 (comment) What's the status of this PR? Is it ready for a final review/approval? |
👍
Yeah it should be ready for a final review/approval. |
What?
This changes the layout type of the field to the field level instead of the form. This way a field can determine its own type.
Why?
We have a use case in the quick edit, where we want to not show a panel layout for some of the fields like images.
We also discussed in the combined edit PR the alternative of moving the layout to the field level.
How?
This adds support to the
form.fieldsarray for this type:Where
FormFieldcan either be a string ( referencing a field definition ) or the above config.Some additional thoughts, how would a
grouplayout be handled here? as a group would include a title. Should this be alayoutwith thetitlerequirement?Testing Instructions
npm run storybook:devand go to the DataForm storypanelview andinlineviewStickyfield is rendered as a inline view and the remainder in either thepanelorinlineview.npm run storybook:devand enable the dataview related experiments in GutenbergTesting Instructions for Keyboard
Screenshots or screencast