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

Grid: Fix percent sizing #306

Merged
merged 12 commits into from
Dec 30, 2022

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Dec 29, 2022

Objective

This PR addresses bugs with percentage sizing (particularly of margins and padding) found when testing the CSS Grid implementation.

Changes made

  • Several new gentests were added around percentage sizing, mostly for grid. One test was added for flexbox which is actually a simplified version of an existing test (which made it easier to debug).
  • A parent_size parameter was added to compute_node_layout all top-level algorithm functions. This is unfortunately necessary to make percentage sizing work correctly as percentage nodes
  • A number of fixes for percentage sizing, making sure that percentages are all resolving against the correct size (or at least fixing several cases where this was previously wrong).

Possible future changes

  • Change the available_space parameter to compute_node_layout to sizing_constraint which a Size<SizingConstraint> where SizingConstraint is enum { MinContent, MaxContent, Definite }. Currently the available_space parameter is duplicating information from the parent_size parameter when it is Definite.
  • Pass parent_size to measure_funcs.

Feedback wanted

Should the future changes block this PR being merged?

@nicoburns nicoburns added the bug Something isn't working label Dec 29, 2022
@nicoburns nicoburns force-pushed the grid/complex-percent-sizing branch from f716b26 to 2c95146 Compare December 29, 2022 22:09
@alice-i-cecile
Copy link
Collaborator

Should the future changes block this PR being merged?

No, IMO we should fix the bug here ASAP and then break things later with these tests in place.

@nicoburns nicoburns force-pushed the grid/complex-percent-sizing branch from fa7e932 to a5e3df3 Compare December 30, 2022 10:52
Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

As far as I can tell this looks good. I'm not able to spot anything obvious, but at the same time I'm not feeling very proficient at this algorithms at the moment.

Passing down ParentSize might actually to support calc() of Dimension? (#225, #232)

src/compute/mod.rs Outdated Show resolved Hide resolved
@@ -687,7 +707,7 @@ fn expand_flexible_tracks(
AvailableSpace::Definite(available_space) => {
let used_space: f32 = axis_tracks.iter().map(|track| track.base_size).sum();
let free_space = available_space - used_space;
if free_space == 0.0 {
if free_space <= 0.0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can free space ever be negative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can yeah. This would be the case if the containing node has a fixed size (set with the width/height property), and the contents of the the container had minimum sizes that sum to total greater than the size of the container (so the contents overflow). I hadn't considered this case either, but it came up when debugging a test, hence the fix!

Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Code quality LGTM, and I trust you + the tests to steer us right here.

@alice-i-cecile alice-i-cecile merged commit 30f8364 into DioxusLabs:main Dec 30, 2022
@nicoburns nicoburns deleted the grid/complex-percent-sizing branch December 30, 2022 22:33
This was referenced Jan 4, 2023
@nicoburns nicoburns added this to the 0.3 "CSS Grid" milestone Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants