vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
469 stars 83 forks source link

Lumo: add theme variant to place fields label on the side #2883

Open web-padawan opened 3 years ago

web-padawan commented 3 years ago

Motivation

For now, to place field labels on the side users have to use vaadin-form-item which has some serious issues:

First issue in this list is the most important one for users: it got more than 30 "đź‘Ť" reactions in total.

Also, in Vaadin Start we have a custom theme which currently relies on unsupported CSS selector e.g

:host([theme~='side-label']) [class$='container'] {
  flex-direction: row;
}

Proposed Solution

Provide a built-in Lumo theme variant for all the field components that could make vaadin-form-item obsolete.

jouni commented 3 years ago

Me and @anezthes have been planning on doing this for some time now, but never got to actually doing it.

I suppose we could implement this with CSS grid layout, so we would not require any additional container elements? It could mean that we need to make the "container" a themable part, unless we move these variants to be "built-in" in the core styles, so that all themes would automatically support them.

This variant should also work together with "helper-above-field", which I think we should rename to "helper-before-field". When paired together with "side-label" (name to be decided), the helper would be below the label.

What do you think?

jouni commented 3 years ago

Ah, bummer. Perhaps CSS grid is not suitable for this after all. It’s not possible to place two elements in the same grid cell/area without them overlapping. That makes it infeasible to place the helper below the label.

web-padawan commented 3 years ago

we move these variants to be "built-in" in the core styles, so that all themes would automatically support them.

I think that could be one option, e.g. by adding labelPosition property and setLabelPosition() Flow API.

jouni commented 3 years ago

Here are mockups from the time we designed the helper feature, comparing the placement (below label, above field, below field).

Screen Shot 2021-10-18 at 16 03 21

knoobie commented 3 years ago

What do you think about having the helper-text be on another row? Like below the label and the field? This would save a lot of horizontal space as well, because those texts could be really long

Edit: Sample grid layout: https://grid.layoutit.com/?id=5tMM7XI

TatuLund commented 3 years ago

There is one version of the idea here: https://cookbook.vaadin.com/textfield-label-left

image

jouni commented 3 years ago

What do you think about having the helper-text be on another row? Like below the label and the field?

Could work for single line inputs, but could also be weird for text areas and radio/checkbox groups:

Screen Shot 2021-10-18 at 16 19 17

knoobie commented 3 years ago

Could work for single line inputs, but could also be weird for text areas and radio/checkbox groups:

Sadly true.. to force a "unified" view we always added helper below the input field; because the space in our applications for label was just 200px; not enough space for long and accuracy descriptions (Germans love long texts)

Example for a "simple last name":

image

DiegoCardoso commented 3 years ago

I like the helper aligned to the field (the last two columns on the mockups). But when the helper is above the field, it looks a bit unbalanced to have three text baselines:

Screenshot 2021-10-18 at 16 28 44

Maybe the label should be align with the field regardless of the helper position.

jouni commented 3 years ago

@DiegoCardoso: yeah, I think you’re right. At least have the helper and label share the same baseline.

TatuLund commented 3 years ago

Example for a "simple last name":

@knoobie is using this add-on component to produce the expandable long helper texts

https://vaadin.com/directory/component/field-description

DiegoCardoso commented 3 years ago

@jouni I am not sure yet if the label should be aligned with the helper or the field.

It looks, to me, that having the label aligned with the field will keep the visual hierarchy across all the fields the same, whether they have a helper text or not. If they are aligned to the helper, when the user clicks on the label, they will have to scan diagonally to find the focused field.

jouni commented 3 years ago

I think it’s a poor choice in any case to have the label on the side and the helper on top of the input. I would not recommend using that.

jouni commented 3 years ago

We talked with @anezthes and agreed that we don't need to support the "side label + helper below label" combination right now. That can remain as a future enhancement. Similarly "side label + helper above field" is a discouraged combination, and doesn't need to work perfectly (alignments).

I wanted to get this moving sooner than later, so I spent some hours today prototyping this in detail.

I think we would want this as a "core" feature, instead of a Lumo (and Material) theme feature. And as we probably agree, this should work together with the existing labelsPosition and CSS custom properties (label width, spacings) API in Form Layout. But it should be usable on a standalone field as well.

With those constraints, I considered @web-padawan’s proposal to be a good one, and tried adding a label-position="aside" attribute on all field components. I tweaked Form Layout to apply that to all child items (where it previously only set that on <vaadin-form-item> elements and only when labelsPosition: "top"). Both Lumo and Material themes work.

You can try the prototype in the feat/field-side-label branch. There’s a new /dev/form-layout.html test page you can check out.

Screen Shot 2021-10-20 at 16 39 34

I did end up implementing the prototype using CSS grid after all, as I didn't have to consider the "helper below label" variant.

What is missing:

jouni commented 3 years ago

As discussed with @rolfsmeds, we don't want to cause more breaking changes in V23, so this behavior should be completely opt-in at this point.

That means there should be some API in Form Layout to enable this new behavior, basically making this code path conditional.

web-padawan commented 3 years ago

Note, this was also requested for Checkbox in #440 - e.g. using checkbox.setLabelPosition(Position.LEFT).

rolfsmeds commented 3 years ago

Note, this was also requested for Checkbox in #440 - e.g. using checkbox.setLabelPosition(Position.LEFT).

I wouldn't consider that the same thing. IMO a checkbox label is more comparable to a field value, so I propose we ignore #440 in this context.

knoobie commented 2 years ago

With the PIT date for 23 upcoming in some weeks - I'm really curious, is this feature still in time for the upcoming v23.0? We heavily rely on this addition for our upcoming migrations and would like to have this in there (a lot of other probably as well if they would know about it).

The change of Jouni looks good, small and non-intrusive, and brings a lot of positive UX and DX. If there is something where I could contribute to get it up and running just hit me (like Java API).

rolfsmeds commented 2 years ago

Hey @knoobie, not sure yet, but we're looking into it. The main issue is that it will affect existing forms in which labelsPosition:aside has been used without FormItem wrapping (that's most FormLayouts actually, since it's the default for breakpoints >=20em), and we'd rather not introduce an API for opt-in.

In case something prevents us from shipping this with 23.0, FormLayout/FormItem has been fixed in terms of label accessibility and required indicators, so you should be able to use that for labels-aside, in case that helps.

knoobie commented 2 years ago

Thanks for looking into this Rolf! Really appreciated!

Affecting in a positive way or? Like now all fields ("native components" and wrapped components with FormItem) would work the same? But I can understand that you wanna reduce the amount of breaking change or APIs that are just added as a workaround.

I'm currently using V22 with the FormItem as workaround, but did not have enough time to check if the empty <label> created by the field components could create problem if a TextField is wrapped by a FormItem and the "outer" label slot of the FormItem is used.

image

rolfsmeds commented 2 years ago

Affecting in the sense that those unwrapped fields will suddenly render with their labels aside instead of on top, which is kind of a big unexpected change. It's probably how you expect it to work when you're building a new form (only to be disappointed when you realize it doesn't work without wrapping into FormItems), but if you already have a big application with tons of forms that have always rendered labels-on-top, you probably don't expect that to suddenly change, and modifying each of those FormLayouts to use labels-on-top for all responsiveSteps can be a lot of work.

rolfsmeds commented 2 years ago

Acceptance criteria (WIP): https://github.com/vaadin/platform/issues/2631