DevTools: MQs should start at zero in the grid

DevTools: MQs should start at zero in the grid

The left hand icon bar now is drawn through to the bottom of the toolbar, so media queries start at actual zero of the rulers. The vertical ruler had to be enlarged to accommodate.

BUG=411247
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181743

[pbakaus, 2014-09-05 11:50:53]

pbakaus@chromium.org changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[dgozman, 2014-09-08 11:44:26]

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:125: flex-grow: inherit;
Why do you need this?

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:473: z-index: 1;
If you want to place media inspector above the canvas, just change their DOM
order (see ResponsiveDesignView constructor).

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:558:
this._toolbarHorizontalContainer = toolbar.createChild("div", "hbox flex-auto");
Why change all this? I don't how this affects anything based on screenshot
attached.

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:580:
this._mediaInspector = new WebInspector.MediaQueryInspector();
You create media inspector here for the second time (first is in the
constructor).

[pbakaus, 2014-09-08 12:02:35]

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:125: flex-grow: inherit;
On 2014/09/08 11:44:26, dgozman wrote:
> Why do you need this?

The "more" icon does not float at the right if I remove it.

https://codereview.chromium.org/544013003/diff/100001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:473: z-index: 1;
On 2014/09/08 11:44:26, dgozman wrote:
> If you want to place media inspector above the canvas, just change their DOM
> order (see ResponsiveDesignView constructor).

Cannot do in this case. The order goes like this:

1) MQ gradient background with inset shadow (from outer MQ DOM element)
2) Rulers drawn from canvas
3) MQs

Thus, the MQ outer element has no zindex and stays at the very bottom, then
comes the rulers on top, and the MQs themselves get a z-index, placing them
above all. There is no other solution I can think of that's simpler, other than
drawing the whole BG in the canvas.

[dgozman, 2014-09-09 14:46:50]

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:497: background:
linear-gradient(to bottom, rgb(119, 203, 28), rgb(4, 166, 0));
It's very hard to see the border between two overlapping green bars now.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:504: background:
linear-gradient(to right, rgba(255, 255, 255, 0.27), rgba(0,0,0,0) 10%),
linear-gradient(to bottom, rgb(222, 129, 30), rgb(204, 104, 31));
This gradient scales when you resize DevTools/browser. I don't really like that.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:556: font-size: 10px;
Don't you think this is too small?

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:191: 
nit: remove this extra line.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:477:
this._mediaInspectorContainer.style.top = cssRulerHeight;
I know this is not your change, but... If you set paddingTop here instead of top
and add "top: 0; overflow: hidden;" to this element in css, serifs will not
overlap the grid when hovering. Right now it looks bad.

[pbakaus, 2014-09-10 11:18:16]

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:497: background:
linear-gradient(to bottom, rgb(119, 203, 28), rgb(4, 166, 0));
On 2014/09/09 14:46:49, dgozman wrote:
> It's very hard to see the border between two overlapping green bars now.

True. Will fix.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:504: background:
linear-gradient(to right, rgba(255, 255, 255, 0.27), rgba(0,0,0,0) 10%),
linear-gradient(to bottom, rgb(222, 129, 30), rgb(204, 104, 31));
On 2014/09/09 14:46:49, dgozman wrote:
> This gradient scales when you resize DevTools/browser. I don't really like
that.

Agreed. Will fix.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:556: font-size: 10px;
On 2014/09/09 14:46:49, dgozman wrote:
> Don't you think this is too small?

Nope, I think that size works really well. The font needs a minimum margin above
and below the bar if it is overlaying and 10px is the sweet spot.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:191: 
On 2014/09/09 14:46:49, dgozman wrote:
> nit: remove this extra line.

Done.

https://codereview.chromium.org/544013003/diff/160001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:477:
this._mediaInspectorContainer.style.top = cssRulerHeight;
On 2014/09/09 14:46:49, dgozman wrote:
> I know this is not your change, but... If you set paddingTop here instead of
top
> and add "top: 0; overflow: hidden;" to this element in css, serifs will not
> overlap the grid when hovering. Right now it looks bad.

Good point, will address.

[dgozman, 2014-09-10 12:41:46]

Almost perfect!

https://codereview.chromium.org/544013003/diff/180001/Source/devtools/front_e...
File Source/devtools/front_end/responsiveDesignView.css (right):

https://codereview.chromium.org/544013003/diff/180001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:485: -webkit-filter:
brightness(65%);
Most of the MQs are inactive on the page, so the whole picture looks too dark
for me. Maybe 80%?

https://codereview.chromium.org/544013003/diff/180001/Source/devtools/front_e...
Source/devtools/front_end/responsiveDesignView.css:559: text-shadow: rgba(0, 0,
0, 0.5) 1px 0;
This is hard to read on min-width bars. Maybe move shadow to the left?

https://codereview.chromium.org/544013003/diff/180001/Source/devtools/front_e...
File Source/devtools/front_end/toolbox/ResponsiveDesignView.js (right):

https://codereview.chromium.org/544013003/diff/180001/Source/devtools/front_e...
Source/devtools/front_end/toolbox/ResponsiveDesignView.js:476:
this._mediaInspectorContainer.style.paddingTop = cssRulerHeight;
Sorry for misleading, but this should be marginTop, so that ruler can keep black
background.

[dgozman, 2014-09-10 15:15:15]

lgtm

[pbakaus, 2014-09-10 15:23:31]

The CQ bit was checked by pbakaus@chromium.org

[commit-bot: I haz the power, 2014-09-10 15:24:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pbakaus@chromium.org/544013003/200001

[commit-bot: I haz the power, 2014-09-10 16:46:50]

Committed patchset #11 (id:200001) as 181743