Fix grid track sizing for `fit-content(percentage)` and for items spanning percentage gaps #335

nicoburns · 2023-01-22T16:29:39Z

Objective

More correctness fixes for #204. This one covers three additional cases:

Content sizing tracks using items spanning 2 track (with fixed sized gaps between)
Content sizing tracks using items spanning 2 track (with percentage sized gaps between)
Sizing tracks with a fix-content track sizing function where the argument to that function is a percentage.

… the tracks

…k sizing rounds WIP

…tation

…a fit-content sizing function

…er than available_grid_space

…perty if growth_limit is infinite

…ite tests

alice-i-cecile · 2023-01-22T23:33:26Z

src/compute/grid/track_sizing.rs

@@ -566,6 +600,12 @@ fn distribute_item_space_to_base_size(
            return;
        }

+        // 0. Prep limit function


This could use a bit more description; the nature and value of this step is fairly unclear when reading the code.

Description added!

alice-i-cecile

Very nice: I'm glad to see the extensive testing paying off. Generally quite clear, but I left a comment in a spot I think could be improved.

…to_base_size

Weibye

I feel like you churn out these PR's faster than I can read them 🚀 It's a luxury problem to have, so keep doing this good work that you do!

The code looks good in my eyes, with the usual caveats about low confidence deep within these algorithms.

Weibye · 2023-01-23T18:33:47Z

test_fixtures/grid_max_content_single_item_span_2.html

+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>
+  <div></div>


Any particular reason why these has a lot of empty divs but the same was removed in a few other tests?

Not really. For some of the other tests (like alignment for example) you do need multiple rows/columns to be able to test properly so I've been using a 3x3 grid. But that becomes annoying when you want to trace through the code execution as everything is repeated 9 times. The ones where the divs have been removed are the ones I had to debug!

Weibye · 2023-01-23T19:07:04Z

src/compute/grid/track_sizing.rs

@@ -360,7 +360,13 @@ fn resolve_intrinsic_track_sizes(
            &get_track_size_estimate,
        );

-        let margin = item.margin.map(|m| m.resolve_or_zero(available_grid_space.width.into_option())).sum_axes();
+        let margin = Rect {
+            left: item.margin.left.resolve_or_zero(Some(0.0)),


I'm curious if it would be possible to rewrite our resolve-systems so that we could write it something like this:

left: item.margin.left.resolve_or(zero()),

And that it would figure out what type the zero() would need to output.

Not sure if it would work, or if it did, if it would improve readability of the codebase at all.

I think that wouldn't actually do quite the same thing. It's actually a coincidence that the argument is zero here. In general the Some(0.0) will be the actual size of the parent node. It's only zero in this case because in this specific scenario horizontal percentage margins always resolve to zero (because otherwise there would be a cyclic dependency).

What this function is doing is:

Returning Points values

Resolving Percent values against the passed in argument (if it's Some)

Else returning 0.0

nicoburns added 17 commits January 22, 2023 15:12

Add tests for child spanning two max-content tracks with gaps between…

5938e39

… the tracks

Recompute available_grid_space and node_inner_size after initial trac…

89576a9

…k sizing rounds WIP

Suspect fix WIP

afc6560

Use computed column sizes for first iteration of row track size compu…

fba9947

…tation

Don't limit the distribution of min-content sizes by the argument of …

7dc85fd

…a fit-content sizing function

Simplify fit-content tests

51bdf5e

Resolve percentage track sizing function against node_inner_size rath…

ffb8ab1

…er than available_grid_space

Add disabled percentage fit-content test

5d9d4fc

Percentage fit content limits WIP

8b208f3

Distribute growth_limit changes (broken)

463bed4

Simplify fit-content percent definite test

b557f6f

Apply fit-content limit to growth limits

6784f8e

When distributing space to growth_limit use base_size as affected pro…

a6afbb3

…perty if growth_limit is infinite

Add min_content and max_content variants of fit-content percent defin…

82040cf

…ite tests

Add percent indefinite fit content tests

3451951

Remove debug logs

0ec28eb

Remove commented out code

c60853b

nicoburns mentioned this pull request Jan 22, 2023

Support CSS Grid #204

Closed

87 tasks

Fix clippy lints

5e27a58

alice-i-cecile added the bug Something isn't working label Jan 22, 2023

nicoburns added this to the 0.3 "CSS Grid" milestone Jan 22, 2023

nicoburns requested review from Weibye and alice-i-cecile January 22, 2023 22:32

alice-i-cecile reviewed Jan 22, 2023

View reviewed changes

alice-i-cecile approved these changes Jan 22, 2023

View reviewed changes

nicoburns added 2 commits January 22, 2023 23:58

Improve documentation of callback functions in distribute_item_space_…

b477f32

…to_base_size

Remove useless code

d023e9c

Weibye approved these changes Jan 23, 2023

View reviewed changes

Merge branch 'main' into grid/percent-fit-content--with-content-sizing

9c7dc89

alice-i-cecile enabled auto-merge (squash) January 23, 2023 20:47

alice-i-cecile merged commit 0596475 into DioxusLabs:main Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix grid track sizing for `fit-content(percentage)` and for items spanning percentage gaps #335

Fix grid track sizing for `fit-content(percentage)` and for items spanning percentage gaps #335

Uh oh!

nicoburns commented Jan 22, 2023

Uh oh!

alice-i-cecile Jan 22, 2023

Uh oh!

nicoburns Jan 23, 2023

Uh oh!

alice-i-cecile left a comment •

edited

Loading

Uh oh!

Weibye left a comment

Uh oh!

Weibye Jan 23, 2023

Uh oh!

nicoburns Jan 23, 2023

Uh oh!

Weibye Jan 23, 2023

Uh oh!

nicoburns Jan 23, 2023

Uh oh!

Uh oh!

Fix grid track sizing for fit-content(percentage) and for items spanning percentage gaps #335

Fix grid track sizing for fit-content(percentage) and for items spanning percentage gaps #335

Uh oh!

Conversation

nicoburns commented Jan 22, 2023

Objective

Uh oh!

alice-i-cecile Jan 22, 2023

Choose a reason for hiding this comment

Uh oh!

nicoburns Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Weibye Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

nicoburns Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

Weibye Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

nicoburns Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix grid track sizing for `fit-content(percentage)` and for items spanning percentage gaps #335

Fix grid track sizing for `fit-content(percentage)` and for items spanning percentage gaps #335

alice-i-cecile left a comment •

edited

Loading