shared-components icon indicating copy to clipboard operation
shared-components copied to clipboard

Field layouts need ids so that <Fields> work inside of <Fields>

Open zacklitzsinger opened this issue 7 years ago • 5 comments

Fields use CSS grid to align their contents; however, they do so by assigning grid template areas that aren't namespaced at all. Instead, these values should be namespaced so that forms can contain other forms. This is useful to provide support for certain UI patterns, and in general is a common source of bugs in form code consuming the SCL.

zacklitzsinger avatar Oct 16 '18 21:10 zacklitzsinger

Nesting <form> elements (if that's what you're talking about) is explicitly called out as unsupported in the HTML5 draft: https://www.w3.org/TR/html5/forms.html#the-form-element

But the library's Form element in general is pretty bad and I rarely use it anymore. It was designed for a different era of field layouts which needed horizontal layout in the form container, so it has a default flex-direction: row style which is really confusing when using it standalone.

If we're going to change Form I'd suggest just deprecating it and suggesting using the regular form tag instead.

a-type avatar Oct 17 '18 12:10 a-type

@a-type Sorry, I meant fields.

zacklitzsinger avatar Oct 17 '18 13:10 zacklitzsinger

I tried some implementations of this and eventually came to the conclusion that there are really only two valid use cases for this: splitting a Field "cell" into multiple columns and using a single column layout where some of the rows are unique components. The ideal way to handle this would allow consumers to create components that use Fields and utilize those components in other Fields (like an Address component using Fields inside another larger form). I think we can still improve this, but we have to consider which use cases we really want to support.

zacklitzsinger avatar Oct 23 '18 21:10 zacklitzsinger

I personally think the Address use case is more correctly modeled using multiple adjacent Field.Group blocks.

<Field.Group columns={1}>
  <Field label="Name">{/* ... */}</Field>
</Field.Group>
<AddressSubFormThing />
<Field.Group columns={1}>
  <Field label="Foo">{/* ... */}</Field>
</Field.Group>

This doesn't work well right now because Fields doesn't have a bottom margin, so these adjacent blocks will be too close together. That could be fixed though, if we wanted to encourage this behavior.

Field semantically is meant to represent a single logical item in a form which has a name (label). Fitting a group of fields into one Field seems like it breaks the semantics to me.

I'm not convinced Field is even the right solution for the problem to begin with though. I wonder if we wouldn't be better off giving users the tools to craft their own grids more easily rather than rely on our layout 'magic' which tends to get abused to produce results which look right, but are difficult to understand semantically or logically.

a-type avatar Nov 08 '18 16:11 a-type

Field is mostly just CSS grid, the extra sauce on top is making sure that labels and help text line up and handling the required/label callout use cases. If we could find an easy way to enable that common use case, then we could nix it entirely.

zacklitzsinger avatar Nov 08 '18 16:11 zacklitzsinger