Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

[css-grid] Add more tests for Implied Minimum Size of Grid Items section #1199

Merged
merged 2 commits into from
Feb 15, 2017

Conversation

mrego
Copy link
Member

@mrego mrego commented Feb 14, 2017

This adds more coverage for that spec section:
https://drafts.csswg.org/css-grid/#min-size-auto

The examples in the tests were discussed in w3c/csswg-drafts#283.

Please @javifernandez or @svillar take a look. Thanks!


This change is Reviewable

This adds more coverage for that spec section:
https://drafts.csswg.org/css-grid/#min-size-auto

The examples in the tests were discussed in w3c/csswg-drafts#283.
@mrego mrego added this to the css-grid-1_dev milestone Feb 14, 2017
@mrego mrego self-assigned this Feb 14, 2017
@syncbot
Copy link
Collaborator

syncbot commented Feb 14, 2017

@syncbot
Copy link
Collaborator

syncbot commented Feb 14, 2017

Automatic validation checks of commit 5c38777 passed.

@mrego
Copy link
Member Author

mrego commented Feb 14, 2017

I thought author field was optional now.

@javifernandez
Copy link

Wouldn't be better to use red as "test-grid-item-overlapping-xxx" background so we see red only it it overflows ?


Review status: 0 of 9 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@javifernandez
Copy link

Reviewed 8 of 9 files at r2.
Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


css-grid-1/grid-items/grid-minimum-size-grid-items-011.html, line 7 at r2 (raw file):

<link rel="help" href="http://www.w3.org/TR/css-grid-1/#min-size-auto" title="6.5. Implied Minimum Size of Grid Items">
<link rel="match" href="../../css21/reference/ref-filled-green-100px-square.xht">
<meta name="assert" content="Checks that automatic minimum size gets clamped, so the grid item doesn't overflow the fixed size area.">

I guess this test will verify the item is stretched to fill the 500px x 500px grid, so does it makes sense to test if it overflows ?


Comments from Reviewable

@mrego
Copy link
Member Author

mrego commented Feb 14, 2017

Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


css-grid-1/grid-items/grid-minimum-size-grid-items-011.html, line 7 at r2 (raw file):

Previously, javifernandez (Javier Fernandez Garcia-Boente) wrote…

I guess this test will verify the item is stretched to fill the 500px x 500px grid, so does it makes sense to test if it overflows ?

No, this patch the grid item is no bigger than 100x100.
It's because of the size of the grid container is 100x100, so the track sizing algorithm resolves the minmax(0px, 500px) track size as 100px.
Then it's considered we've a fixed track, so the automatic minimum is clamped to 100px (instead of the 200px of its content).


Comments from Reviewable

@mrego
Copy link
Member Author

mrego commented Feb 15, 2017

I could change that, that will show "red" if the item overflows.
But what would happen if the item is smaller than 100x100, it'd be hidden by the positioned "green" element. So we won't be detecting those issues.
It's true that these tests are checking overflow, not the other thing, but I prefer that the test detects both mistakes that only one. If you don't mind I prefer to keep them as they're now.
What do you think?


Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@javifernandez
Copy link

Ok, I understand.


Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@javifernandez
Copy link

:lgtm:


Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@javifernandez
Copy link

Reviewed 1 of 9 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@mrego mrego merged commit c7e571d into w3c:master Feb 15, 2017
@mrego mrego deleted the new-minimum-size-tests branch February 15, 2017 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants