RESOLVED FIXED 107044
[CSS Grid Layout] Add grid.css to hold the common grid testing code 
https://bugs.webkit.org/show_bug.cgi?id=107044
Summary [CSS Grid Layout] Add grid.css to hold the common grid testing code
Julien Chaffraix
Reported 2013-01-16 12:41:10 PST
Good suggestion from Tony! This should be similar to what is done in flexbox.css. The idea is to centralize some of the definitions to avoid having to touch lots of files when unprefixing or updating the syntax (e.g. see bug 103336 and bug 103338).
Attachments
Proposed change 1: Added grid.css and move most files to it. (36.12 KB, patch)
2013-01-18 17:10 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Proposed change 2: Updated the class names per Tony's request. (75.08 KB, patch)
2013-01-22 12:58 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Proposed change 3: s/id/class/g. (75.30 KB, patch)
2013-01-22 14:49 PST, Julien Chaffraix 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Julien Chaffraix
Comment 1 2013-01-18 17:10:07 PST
Created attachment 183578 [details] Proposed change 1: Added grid.css and move most files to it.
Julien Chaffraix
Comment 2 2013-01-20 14:03:17 PST
*** Bug 104869 has been marked as a duplicate of this bug. ***
Tony Chang
Comment 3 2013-01-22 10:17:29 PST
Comment on attachment 183578 [details] Proposed change 1: Added grid.css and move most files to it. View in context: https://bugs.webkit.org/attachment.cgi?id=183578&action=review > LayoutTests/fast/css-grid-layout/resources/grid.css:11 > +#a { I think using single letter ids seems like it could cause problems and weird side effects in the future. Can we make these classes with descriptive names?
Julien Chaffraix
Comment 4 2013-01-22 12:58:34 PST
Created attachment 184037 [details] Proposed change 2: Updated the class names per Tony's request.
Tony Chang
Comment 5 2013-01-22 13:48:34 PST
Comment on attachment 184037 [details] Proposed change 2: Updated the class names per Tony's request. View in context: https://bugs.webkit.org/attachment.cgi?id=184037&action=review > LayoutTests/fast/css-grid-layout/resources/grid.css:11 > +#firstRowFirstColumn { These should be class names, no? You have test cases that have multiple divs with the same id, which is confusing.
Julien Chaffraix
Comment 6 2013-01-22 14:42:13 PST
Comment on attachment 184037 [details] Proposed change 2: Updated the class names per Tony's request. View in context: https://bugs.webkit.org/attachment.cgi?id=184037&action=review >> LayoutTests/fast/css-grid-layout/resources/grid.css:11 >> +#firstRowFirstColumn { > > These should be class names, no? You have test cases that have multiple divs with the same id, which is confusing. Not really, the tests have had several divs with the same id for some time. From a CSS selector perspective, it is fine. Where it will get hairy is if you try to query the id from JavaScript. I agree that this should be changed and I might as well pile it on this patch.
Julien Chaffraix
Comment 7 2013-01-22 14:49:49 PST
Created attachment 184051 [details] Proposed change 3: s/id/class/g.
WebKit Review Bot
Comment 8 2013-01-22 15:34:47 PST
Comment on attachment 184051 [details] Proposed change 3: s/id/class/g. Clearing flags on attachment: 184051 Committed r140480: <http://trac.webkit.org/changeset/140480>
WebKit Review Bot
Comment 9 2013-01-22 15:34:51 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.