Skip to content

Update LayoutCard story in DataForm to use card layout #73695

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.

Sign up for GitHub

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

Merged
oandregal merged 1 commit into from
Dec 23, 2025
Merged

Update LayoutCard story in DataForm to use card layout #73695

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/dataviews/src/stories/dataform.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1575,6 +1575,7 @@ const LayoutCardComponent = ( {

const form: Form = useMemo(
() => ( {
layout: { type: 'card' },
Copy link
Member

@oandregal oandregal Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 This should not be necessary because every field declares its layout, and all the 3 top-level fields that create a card have card as the layout. That should be enough to match any styles. I'll take a look.

Copy link
Contributor Author

@jameskoster jameskoster Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

In case it's not clear, this is about the spacing between the cards themselves (which should be 24px), not the fields within the cards (which should be 16px).

Copy link
Member

@oandregal oandregal Dec 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wrapper layout depends on the form.layout.type. If not present, we normalize it to the regular layout that uses 4 as spacing setting, while the card layout uses 6 (ref).

This is fragile. 🤔

Copy link
Member

@oandregal oandregal Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it was the individual fields (each card, each panel, each regular field) that set the spacing? Something along these lines:

  <div class="container">
    <div class="card">...</div>
    <div class="panel">...</div>
    <div class="regular">...</div>
  </div>
// DataForm CSS
  .container {
    display: flex;
    flex-direction: column;
  }

// The card layout's CSS
  .card + * {
    margin-top: 24px;
  }

// The panel layout's CSS
  .panel + * {
    margin-top: 16px;
  }

// The regular layout's CSS
  .regular + * {
    margin-top: 8px;
  }

Would there be any issue with this? We could remove all the wrappers from the existing layouts but the row one.

Copy link
Contributor Author

@jameskoster jameskoster Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think something along those lines might work. I made a CodePen to explore. The rules are a bit more specific to account for specific order combos but I think this covers everything neatly.

Do you think there are any trade-offs to using margin vs gap?

Copy link
Contributor Author

@jameskoster jameskoster Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oandregal any thoughts on this?

Copy link
Member

@oandregal oandregal Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've approved this PR, but we can continue the conversation.

I thought about this, but I've noticed we mix panel and regular items in a few places (QuickEdit, for example). If the field themselves provide the spacing, and each layout has its own, wouldn't this create different spacing between fields in the same form?

Copy link
Member

@oandregal oandregal Dec 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought is that we make it a card because we want the spacing of a card. The spacing applied between fields is overloaded with the layout type. A different approach could be 1) remove the top-level layout prop, it's the field responsibility to declare its own 2) and a top-level gap prop to configure the spacing between fields.

If we go this route, I suppose the best approach would be to connect this gap with the spacing from DS (e.g., "xs", "md").

fields: [
{
id: 'customerCard',
Expand Down
Loading