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.

Sign up for GitHub

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

Jump to bottom

[css-grid-1] Step 2.2 of Distribute Extra Space confusingly written for growth limits #5351

Closed
fantasai opened this issue Jul 22, 2020 · 7 comments
Closed

[css-grid-1] Step 2.2 of Distribute Extra Space confusingly written for growth limits #5351

fantasai opened this issue Jul 22, 2020 · 7 comments
Labels
Closed Accepted as Editorial Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1

Comments

@fantasai
Copy link
Collaborator

https://drafts.csswg.org/css-grid-1/

Distribute space to base sizes up to growth limits: Find the item-incurred increase for each spanned track with an affected size by: distributing the space equally among such tracks, freezing a track’s item-incurred increase as its affected size + item-incurred increase reaches its growth limit (and continuing to grow the unfrozen tracks as needed). If a track was marked as infinitely growable for this phase, treat its growth limit as infinite for this calculation (and then unmark it).
Note: If the affected size was a growth limit, each item-incurred increase will be zero.

So this entire step effectively applies only to base sizes, not to growth limits, since all it does to growth limits is set them to zero. It would be nice if the editorial structure was clearer about this up front.
tabatkins added a commit that referenced this issue Jul 22, 2020
@tabatkins
[css-grid-1][editorial] Make initial calculation of item-incurred inc…
98d41e8 
…rease easier to read. #5351
@fantasai fantasai added Closed Accepted as Editorial Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Jul 22, 2020
@fantasai fantasai reopened this Jul 22, 2020
tabatkins added a commit that referenced this issue Jul 22, 2020
@tabatkins
[css-grid-1] Revert but reword step 2 of Distributing Extra Space, as…
776a1fd 
… it *does* have an effect on growth limits if they're infinitely growable. #5351
@fantasai
Copy link
Collaborator Author

Actually this is incorrect, for infinitely-growable tracks this step does have an effect. The note is also wrong, therefore. Let's revise this again. :)

@fantasai
Copy link
Collaborator Author

fantasai commented Jul 23, 2020 •
edited

Fixed in 776a1fd (linked above; Tab pushed our edits before I could post the comment)

@fantasai
Copy link
Collaborator Author

fantasai commented Jul 23, 2020 •
edited

Final change is diff-marked in https://drafts.csswg.org/css-grid-1/#change-2017-distribute-space-readability and should be 100% editorial.

@Loirooriol Mind checking to make sure we didn't screw anything up? :)

fantasai added a commit that referenced this issue Jul 23, 2020
@fantasai
[css-grid-1] Resimplify note. #5351
af5e329
@Loirooriol
Copy link
Contributor

I will review tomorrow, but my understanding was that if the affected size was a growth limit, then the item-incurred increase would be zero, even if the track was marked as infinitely growable.

The infinitely growable flag would only matter when running the spanning algorithm with the next span amount, which distributes into base sizes again.

@Loirooriol
Copy link
Contributor

OK, I had a misconception because this step wasn't right, so it's good that you fixed it :)
The new text works well with Peter Salas' explanation of infinitely growable in https://drafts.csswg.org/css-grid/#infinitely-growable, and also seems to match what Chromium and Firefox implemented.
LGTM

@Loirooriol
Copy link
Contributor

Well, just a nit: step 2 is now called "Distribute space up to limits". Then step 3 should maybe be called "Distribute space beyond limits".

@fantasai
Copy link
Collaborator Author

@Loirooriol Good catch. :) Fixed.

fantasai added a commit that referenced this issue Jul 31, 2020
@fantasai
[css-grid-1] Adjust heading to match previous. #5351 (comment)
e8bdbe7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Closed Accepted as Editorial Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fantasai @Loirooriol