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 a default min-height to grid #7964

Merged
merged 9 commits into from
Oct 14, 2024
Merged

Conversation

sissbruecker
Copy link
Contributor

@sissbruecker sissbruecker commented Oct 10, 2024

Adds a default min-height to the grid to prevent it from collapsing to zero height when using height: 100% in a container that does not have an explicit height. The min-height is calculated from the height of the header, footer and a single grid row. To keep things simple, the row height is hard-coded for now to match the height of a single default row in Lumo, instead of reading it from a CSS property or having a different value for Material. The idea here is that this only needs to be good enough to make a developer notice that their CSS is not set up correctly, rather than to have a correct sizing mechanism.

Closes #4584

Warning

This is a behavior altering change, as intentionally collapsing a grid to zero height now also requires setting the min-height style.

@sissbruecker sissbruecker changed the title feat: add min-height to grid feat: add a default min-height to grid Oct 10, 2024
@sissbruecker sissbruecker marked this pull request as ready for review October 11, 2024 07:43
Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

I noticed that the full row might not be visible in case there's a horizontal scrollbar on the grid:
image

I think it's ok since taking into account the scrollbar detection plus calculating its size might not be trivial for what we want to offer here. Also, there would be some height changes whenever the user or the app changes the grid width so that the horizontal scrollbar appears/hides.

Another thing: should this PR be marked as a breaking change? Setting height to 0 works differently with this change and if the user desires to set the grid height to 0, they need to use min-height instead.

@sissbruecker
Copy link
Contributor Author

@DiegoCardoso Good points. The scroll bar seems tricky to account for, and the assumption is that we only need to be able to show something, not make it particulary usable. We could maybe increase the space reserved for the body a bit so that you see more than a scroll bar in case there are no header or footer. I'll bring it up in the Daily.

Regarding the breaking change, yes I think it makes sense to tag the PR at least.

@sissbruecker sissbruecker changed the title feat: add a default min-height to grid feat!: add a default min-height to grid Oct 14, 2024
Copy link

@sissbruecker
Copy link
Contributor Author

@DiegoCardoso I included the scrollbar into the measurement as suggested by @tomivirkki. Seems to work well on all systems I tested, though there might be some small layout shift in some cases. Mind taking another look?

@sissbruecker sissbruecker removed the request for review from vursen October 14, 2024 08:05
Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

Checked the latest changes in Mac and Windows and the height calculation correctly uses the scrollbar size when it's present.

@sissbruecker sissbruecker merged commit 1cc5372 into main Oct 14, 2024
9 checks passed
@sissbruecker sissbruecker deleted the feat/grid-min-height branch October 14, 2024 10:46
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.6.0.beta1 and is also targeting the upcoming stable 24.6.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply a default min-height for Grid to avoid zero-height situations
4 participants