DataForm: add new card layout
#71100
Conversation
|
Size Change: +818 B (+0.04%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
gigitux
force-pushed
the
add/dataviews-card-layout
branch
from
fc5f900 to
07e5538
Compare
|
@oandregal let me know your thoughts on this and if everything makes sense. Tomorrow I want to try to implement the |
mtias
added
the
[Feature] DataViews
|
Hey @gigitux, thanks for implementing the changes we discussed yesterday. I feel direction is good — just removing "innerLayout" nesting and using the fields instead is much clearer. This approach extends what we have and accommodates potential future layouts (e.g., rows and others). I've pushed some changes to storybook to cover/highlight more of the combinations we talked about. We only need to clarify/fix some edge cases, I'll leave comments in place for them. |
| @@ -0,0 +1,20 @@ | |||
| export type LayoutType = 'regular' | 'panel' | 'card'; | |||
There was a problem hiding this comment.
Perhaps splitting types into files would be good. However, if we do so, I'd rather consider higher-level entities: DataViews types, Field types, DataForm types. I'd add these types in src/types.ts for now.
There was a problem hiding this comment.
Yeah, we need to split the types. The file is big. For now, I moved them in src/types.ts
| type: 'card'; | ||
| labelPosition?: Extract< LabelPosition, 'top' | 'none' >; | ||
| opened?: boolean; |
There was a problem hiding this comment.
labelPosition: I understand where this is coming from, but this is a boolean camouflaged as a set. The semantics of this prop are different from the semantics of labelPosition in regular and panel layouts, and so I think its name should reflect that. Some options: withHeader, hasHeader, etc. Specially considering that the layout object needs to have different props per layout type given its different characteristics (opened is not translatable to regular/panel layouts).
opened: this is a small nit. isOpened would better communicate that is a boolean.
There was a problem hiding this comment.
I've seen designs with and without a collapsible header. If we were to support both, I think we could just add a new isCollapsible prop in the CardLayout to control that specific aspect. No need to do anything in this PR, just wanted to share for our future selves.
There was a problem hiding this comment.
It makes sense. I used isOpened and withHeader 👍
| ( { | ||
| layout: { | ||
| type: 'card', | ||
| // labelPosition: 'none', |
There was a problem hiding this comment.
Note this bug: if I set labelPosition: none the 1st and 3rd card still display the header. The second doesn't (whether or not the field overrides, as I've added). I haven't checked implementation yet, but I suppose this is because we're checking if the card has a label. I think the labelPosition should trump label: this is something that would be clearer the name was something like withHeader (see other comment).
There was a problem hiding this comment.
I fixed the code. This is how the story looks:
To keep the logic simple, if a field specifies a layout but omits some keys, those keys fall back to the default values of that layout type, not to form.layout.
For example, even if form.layout.withHeader is false, the taxConfiguration field has the header.
There was a problem hiding this comment.
To keep the logic simple, if a field specifies a layout but omits some keys, those keys fall back to the default values of that layout type, not to form.layout.
👍 I think this is also easier to reason about when you are a form author.
There was a problem hiding this comment.
I've tested nesting layouts a bit more and it works as expected. It doesn't mean it creates "good designs" (see examples about nesting a card within a card), it means this is ready for when we introduce more container-like layouts (e.g., tabs, rows, etc.) that need nesting.
Some examples include nesting cards within other cards, panel, or regular layouts:
| label: 'Taxes', | ||
| layout: { | ||
| type: 'card', | ||
| opened: false, // TODO: it does not work. |
There was a problem hiding this comment.
I've tried to collapse this card, and it didn't work.
| { id: 'order', layout: { type: 'card' } }, | ||
| { | ||
| id: 'authorDateCard', | ||
| label: 'Author & Date', | ||
| layout: { | ||
| type: 'card', | ||
| }, | ||
| children: [ 'author', 'date' ], | ||
| }, |
There was a problem hiding this comment.
Note this: both the single field order and the combined field authorDateCard set the layout to card without setting anything else. However, order gets the label rendered for its control, but authorDateCard doesn't for its children:
I know it is doable to configure it, and why it happens (it's optimized for the panel layout). But a better default in this case would be the opposite: cards with a single field don't get the control's label displayed, and cards with multiple fields do. Alternatively, defaulting to always display the label sounds fine as well because cards with many controls will be more common than cards with a single control.
| opened: false, // TODO: it does not work. | ||
| }, | ||
| children: [ | ||
| // TODO: default values. |
There was a problem hiding this comment.
Related to the comment at https://github.com/WordPress/gutenberg/pull/71100/files#r2262380433
|
Thanks for confirming the direction! I'm working on clean up the code and address your feedback. I want to get your tought on this: based on this configuration 
Should the |
I added my opinion in this comment. |
|
The only weird thing that I noticed is that with 
The card can't be expanded because the “collapse” behavior is attached to the header. I would not focus too much on this in this PR. We can iterate on it. |
The combination is possible, so we need to cover this case, even if it's a mistake on the consumer side. Can we return null, so no component is rendered? |
This can be more confusing, especially if the consumer marks the field as required. The form would be invalid, but the reason might not be obvious to the consumer. Can we refine the type so that if |
|
Flaky tests detected in 13e7318. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16935682561
|
|
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. |
I've updated the types as suggested, but also prevented this from happening by keeping the card open if the header is not present. |
oandregal
force-pushed
the
add/dataviews-card-layout
branch
from
13e7318 to
d430080
Compare
There wasn't any check on specific layout types, and invalid type (e.g., storybook's "default") were passed down.
oandregal
force-pushed
the
add/dataviews-card-layout
branch
from
9f8cb83 to
f9e1609
Compare
| typeof child === 'string' || ! isCombinedField( child ) | ||
| ); | ||
|
|
||
| if ( simpleChildren.length === 0 ) { |
There was a problem hiding this comment.
This fixes a bug with the panel layout that threw an error at runtime when it didn't have any children that were not "combined fields".
| fields: [ { id: field.id } ], | ||
| }; | ||
| }, [ field ] ); | ||
|
|
There was a problem hiding this comment.
This is a simplification: the form field normalization is already performed by DataFormLayout.
| } | ||
|
|
||
| const labelPosition = field.labelPosition ?? 'side'; | ||
| const layout: NormalizedPanelLayout = normalizeLayout( { |
There was a problem hiding this comment.
Centralized every layout normalization in a single place because there were bugs related to having different defaults per layout (depending on the field being at the top-level or nested). I think this can be improved further because the types and the runtime behavior are out of sync: the fields are already normalized at this point. It was a bit involved to change the pre-existing types, so this can be a follow-up.
What
This PR introduces a new card layout for the DataForm component in the
@wordpress/dataviewspackage. The card layout provides a collapsible, card-based UI for organizing form fields with improved visual hierarchy and user experience.How
Added
'card'as a new layout type alongside existing'regular'and'panel'layouts.Layout definition
formandfieldaccepts alayoutproperty that it is an object:This allows building complex interfaces in a declarative approach by defining different layout types at multiple levels:
Form-level layout (first layer): The form.layout property sets the default layout for all fields in the form. This affects how top-level fields are rendered. Certain layouts include nesting safeguards — for example, Card Layout automatically prevents child cards by defaulting children to 'regular' layout, avoiding problematic infinite nesting scenarios.
Field-level layout overrides: Individual fields can override the form layout by specifying their own layout property, allowing mixed layouts within the same form.
Nested layout inheritance: When using combined fields (fields with children), each child can have its own layout that overrides its parent's layout.
Examples
Check the storybook for more complex ones.
How to test
Run the storybook locally and interact with the
LayoutCardandLayoutMixedstories:npm install && npm run storybook:dev