Skip to content
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.

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

Implement FormLayout with CSS Grid #8583

Open
yuriy-fix opened this issue Jan 28, 2025 · 4 comments · May be fixed by #8590
Open

Implement FormLayout with CSS Grid #8583

yuriy-fix opened this issue Jan 28, 2025 · 4 comments · May be fixed by #8590
Assignees
Labels
enhancement New feature or request vaadin-form-layout

Comments

@yuriy-fix
Copy link
Contributor

yuriy-fix commented Jan 28, 2025

Implement current FormLayout component's behaviour using CSS grid while maintaining backward compatibility.

  • Container queries are not supported in Safari 15, requiring a JS-based fallback
  • Task doesn't include the label positioning (aside), which is out of scope.
  • Can be marked as refactor!:

Follow the AC defined: vaadin/platform#7172

@vursen
Copy link
Contributor

vursen commented Jan 30, 2025

Here are some findings that we made with @DiegoCardoso during the implemetation:

  1. Major: The label-position attribute on FormItem still needs to be supported because users can reference it in their styles, as suggested in the documentation. This makes using container queries not practical at this point as they would only be used to set _grid-cols which is something that can be done in the same place where FormLayout currently updates the label-position attribute on FormItems, reducing the number of solutions we will have to maintain
  2. Major: --vaadin-form-item-row-spacing is currently documented as a FormItem property, which also aligns with its name. Refactoring FormLayout to CSS gaps will require setting this property on FormLayout instead, whereas setting it on FormItem will no longer have any effect.
  3. Minor: Previously colspan was parsed from float to int using Math.floor but CSS grid-column-end seems to use round instead
  4. Question: Should we continue supporting <br />?

@rolfsmeds
Copy link
Contributor

  1. Let's not use container queries in V24.7, let's use JS instead for now and apply label-position attribute, but deprecate its use for styling purposes (jsdoc comment is likely sufficient for that). (In V25 we can probably refactor to use container queries which means that the property will no longer be set on FormItems.)
  2. Introduce --vaadin-form-layout-row-spacing implemented using grid row gap; continue supporting --vaadin-form-item-row-spacing for backward compatibility, implemented using margin/padding in FormItem, but deprecate it (usage should show warning).
  3. Let's ignore this difference – I doubt anybody supplies non-integer colspans
  4. We need to continue supporting <br/>. Set display:none to prevent it taking a grid cell.

@knoobie
Copy link
Contributor

knoobie commented Jan 30, 2025

Just asking: Did you consider creating vaadin-form and deprecate the "old" layout instead of refactoring? It could theoretically simplify implementation and also helps people to migrate.. I've got a gut feeling that people have used the old form layout in such weird ways that even trying out 24.7 could break 2/3 of their forms.. with a new component.. people could migrate one form at a time.

@rolfsmeds
Copy link
Contributor

Yes, it was considered, but

  1. We'd prefer not to have multiple form layout components, and
  2. During prototyping it turned out to be fairly easy to recreate the old behavior using css grid.

We're aware that there are apps out there that do weird things with the current FormLayout that the refactored model may break, but it feels like a slipper slope to avoid changing internal implementation details to avoid breaking custom hacks – we could hardly make any changes if we always tried to avoid that.

If there's a lot of backlash about the changes in 24.7, it's also possible to reintroduce the flexbox-based model in the same component without too much hassle, as it would only be used for responsiveSteps-based usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vaadin-form-layout
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants