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

Apply a default min-height for Grid to avoid zero-height situations #4584

Closed
rolfsmeds opened this issue Sep 13, 2022 · 10 comments · Fixed by #7964
Closed

Apply a default min-height for Grid to avoid zero-height situations #4584

rolfsmeds opened this issue Sep 13, 2022 · 10 comments · Fixed by #7964
Assignees

Comments

@rolfsmeds
Copy link
Contributor

Describe your motivation

Placing a vaadin-grid with height:100% in a container without a defined height tends to result in the component rendering with a height of 0px. While this is in itself not a bug, it can be quite confusing for developers since the Grid renders effectively invisible on the page. This problem is somewhat unique to Grid, in the sense that most, if not all, other Vaadin components do have some kind of implicit min-height.

Describe the solution you'd like

Apply a small default min-height on the Grid's root element, e.g. equivalent to 1 header row and 1 body row, so that developers can see that the Grid does render, and have a better chance of figuring out why it doesn't layout as expected.

Describe alternatives you've considered

No response

Additional context

No response

@rolfsmeds
Copy link
Contributor Author

On second thought, I think the following min-height behavior would make more sense:

headers + body-row + footers

where

  • headers = the combined height of all header rows (if any)
  • body-row = the height of one body row (either the first row, if there is one, or the default body row height, if there isn't, or the empty state template, if one has been defined)
  • footers = the combined height of all footer rows (if any)

This is significantly more complicated to implement, but seeing as how clipped header and footer rows cannot be made visible through scrolling, it would make sense for them to always be included in the min-height.

@rolfsmeds
Copy link
Contributor Author

In order to make it possible to override the default min-height (by applying an inline style to vaadin-grid), and not affect the min-height getter value in Flow, this should be implemented not as an inline style on the root element, but e.g.

  • A generated inline style on the <div id="scroller"> inside, or
  • A custom property with a generated value, applied to the root element like :host() { min-height: var(--vaadin-grid-min-height); } which would be overridden by an inline min-height style.

@tomivirkki
Copy link
Member

Did some prototyping in this branch

Screenshot 2024-05-10 at 12 22 32

The prototype applies the min-height style to the host element so that it can be overridden even to a smaller value in the application styles, for example:

vaadin-grid {
  min-height: 0;
}

It uses constructable stylesheets for the purpose to avoid adding a confusing inline style such as --_grid-min-height on the <vaadin-grid> host element.

@rolfsmeds
Copy link
Contributor Author

Right, it should of course be possible to override it, so a style on the root applied as constructable makes sense.

How come the empty body row is taller than an actual body row?

@tomivirkki
Copy link
Member

How come the empty body row is taller than an actual body row?

When there's no actual data for the rows yet, there's nothing to measure and it falls back to some default row height (I used 40px in the proto). Once the grid has data items, it's possible to measure and use the actual rendered height of the first row.

@rolfsmeds
Copy link
Contributor Author

Right. Would it make sense to do some custom-property-fu here so that it could use --lumo-size-m when using Lumo?

@tomivirkki
Copy link
Member

tomivirkki commented May 27, 2024

Sure, we could use a custom property for it (and apply the min-width with calc).

@yuriy-fix yuriy-fix removed the requires new major This would be a breaking change label Jun 25, 2024
@mshabarov mshabarov added this to Roadmap Jul 1, 2024
@mshabarov mshabarov moved this to September 2024 (24.5) in Roadmap Jul 1, 2024
@yuriy-fix
Copy link
Contributor

@tomivirkki , is this done already?

@tomivirkki
Copy link
Member

No, the prototype is there but it hasn't been worked on further

@sissbruecker sissbruecker self-assigned this Oct 9, 2024
@sissbruecker
Copy link
Contributor

I'd suggest to always use a fixed value for the body part:

  • Measuring the first row could break existing layouts in cases where rows can be extremely large. Then the newly added min-height could extend what developers have currently set as height.
  • I'd also ignore the empty state component to keep things simple. We don't need to build something production ready with this, only an indication for developers that their CSS is borked.

The fixed value should probably be --lumo-size-m, so 36px, which is the height of a single grid row by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: September 2024 (24.5)
Development

Successfully merging a pull request may close this issue.

4 participants