LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html - Issue 543493004: New Test for 'vertical-align has no effect on a grid item'

Side by Side Diff: LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items.html

Issue 543493004: New Test for 'vertical-align has no effect on a grid item' (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 6 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <link href='resources/grid.css' rel='stylesheet'>

	4 <style>

	5 .grid {

	6 background-color:lightgrey;
	Julien - ping for review 2014/09/10 22:18:36 Nit: I like to put a space after the semi-colon li Nit: I like to put a space after the semi-colon like in English. Sunil Ratnu 2014/09/11 12:38:02 I've removed background-color in the latest patch. Show quoted text On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote: > Nit: I like to put a space after the semi-colon like in English. I've removed background-color in the latest patch. Should we keep it, keeping in mind the option of providing more clarity to the user if someone wants to use this code to test separately. What do you think? Julien - ping for review 2014/09/11 18:24:20 I am fine with some colors to ease visual debuggin Show quoted text On 2014/09/11 12:38:02, Sunil Ratnu wrote: > On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote: > > Nit: I like to put a space after the semi-colon like in English. > > I've removed background-color in the latest patch. Should we keep it, keeping in > mind the option of providing more clarity to the user if someone wants to use > this code to test separately. What do you think? I am fine with some colors to ease visual debugging of the test.
	7 }

	8

	9 #item4 {
	Julien - ping for review 2014/09/10 22:18:37 With that declaration, only one grid item is actua With that declaration, only one grid item is actually testing what you want. You can make it match all the grid-items: .grid > * { ... } Or even have different values for vertical-align for increased coverage. Sunil Ratnu 2014/09/11 12:38:02 Done. Show quoted text On 2014/09/10 22:18:37, Julien Chaffraix - PST wrote: > With that declaration, only one grid item is actually testing what you want. > > You can make it match all the grid-items: > > .grid > * { ... } > > Or even have different values for vertical-align for increased coverage. Done.
	10 vertical-align:20px;

	11 }

	12 </style>

	13
	Julien - ping for review 2014/09/10 22:18:36 The test should state what it's testing (aka descr The test should state what it's testing (aka description) and what the expectations are (what is a passing / failing output). Sunil Ratnu 2014/09/11 12:38:01 Done. Show quoted text On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote: > The test should state what it's testing (aka description) and what the > expectations are (what is a passing / failing output). Done.
	14 <div class='grid'>

	15

	16 <!-- grid item: block child -->

	17 <div id='item1'>block</div>

	18

	19 <!-- grid item: floated element; floating is ignored -->
	Julien - ping for review 2014/09/10 22:18:36 This seems like it's testing something unrelated t This seems like it's testing something unrelated to what we want. Sunil Ratnu 2014/09/11 12:38:02 Removed "floating is ignored" but keeping the styl Show quoted text On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote: > This seems like it's testing something unrelated to what we want. Removed "floating is ignored" but keeping the style as float so as to increase the test coverage. Julien - ping for review 2014/09/11 18:24:20 What do you call increased test coverage? If float Show quoted text On 2014/09/11 12:38:02, Sunil Ratnu wrote: > On 2014/09/10 22:18:36, Julien Chaffraix - PST wrote: > > This seems like it's testing something unrelated to what we want. > > Removed "floating is ignored" but keeping the style as float so as to increase > the test coverage. > What do you call increased test coverage? If float is ignored (and it should), then it is identical to .item1. Furthermore, this test is about testing vertical-align, not float. Let's just focus on what we are testing here and remove this case.
	20 <div id='item2' style='float: left;'>float</div>

	21

	22 <!-- grid item: anonymous block box around inline content -->

	23 anonymous item 3

	24

	25 <!-- grid item: inline child -->

	26 <span id='item4'>item 4</span>

	27 </div>

OLD	NEW

« no previous file with comments | « no previous file | LayoutTests/fast/css-grid-layout/vertical-align-do-not-effect-grid-items-expected.html » ('j') | no next file with comments »