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

feat: add layout improvements behind feature flag #8552

Merged
merged 13 commits into from
Jan 31, 2025

Conversation

sissbruecker
Copy link
Contributor

@sissbruecker sissbruecker commented Jan 23, 2025

Adds the following improvements to vaadin-horizontal-layout and vaadin-vertical-layout:

  • When using setFullWidth, setFullHeight or setFullSize on a layout child, users typically expect those to take up the remaining space in the layout, rather than the full size of the layout. In that case automatically apply flex: 1 to allow them to grow and shrink as necessary.
  • Nested layouts that are supposed to take up the remaining space may not shrink because the size of their contents is larger than the remaining space in the layout. So specifically for other layout components, also apply min-width: 0 to allow them to shrink below the size of their contents.

The improvements are behind a feature flag as they may alter existing layouts, so for now we allow users to try them out.

Part of vaadin/flow-components#6998

Comment on lines 35 to 38
::slotted([style^='width: 100%']),
::slotted([style*=' width: 100%']) {
flex: 1;
}
Copy link
Contributor Author

@sissbruecker sissbruecker Jan 23, 2025

Choose a reason for hiding this comment

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

The initial plan was to have a style rule apply to all children, and set flex to the value of a custom CSS property which would have some default value. HasSize.setFullWidth and others would then have set the custom property to 1.

However, applying the rule to all children resulted in overwriting defaults that a component might have defined in their shadow root styles (e.g. grid, virtual list). So we went with this approach, which also makes the solution a bit less cluttered as it doesn't require changes in Flow.

Copy link
Contributor

@vursen vursen Jan 27, 2025

Choose a reason for hiding this comment

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

Would it be less intrusive to override setFullWidth to set flex: 1 instead? I'm concerned that this behavior may not add much value for web component developers and could instead cause confusion, as it makes width behave differently depending on where it's defined.

(My concern is based on the assumption that this feature will become the default in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to scope this change to children of the horizontal and vertical layout components. I'm not sure how we would accomplish this with setFullWidth, apart from the CSS custom property solution that we discarded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to change the approach to target users using HasSize specifically. As such, we don't target styles like width: 100% / height: 100% anymore. Instead, HasSize will add data attributes to the element that the layout components can target for applying additional styles. I've updated the PR accordingly.

Comment on lines 40 to 43
::slotted(vaadin-horizontal-layout),
::slotted(vaadin-vertical-layout) {
min-width: 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly, changing the min size on all children is not desirable. For example, for grid we explicitly added a min-height in the shadow root styles, which should not be overridden by the layout. Other components have an explicit min size as well. As the problem with children not being able to shrink due to their content mostly involves nested layouts we chose to limit this to these two components for now.

@sissbruecker sissbruecker marked this pull request as ready for review January 23, 2025 07:40
Comment on lines 35 to 38
::slotted([style^='width: 100%']),
::slotted([style*=' width: 100%']) {
flex: 1;
}
Copy link
Contributor

@vursen vursen Jan 27, 2025

Choose a reason for hiding this comment

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

Would it be less intrusive to override setFullWidth to set flex: 1 instead? I'm concerned that this behavior may not add much value for web component developers and could instead cause confusion, as it makes width behave differently depending on where it's defined.

(My concern is based on the assumption that this feature will become the default in the future)

@sissbruecker sissbruecker merged commit fa0c544 into main Jan 31, 2025
9 checks passed
@sissbruecker sissbruecker deleted the feat/layout-improvements branch January 31, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants