[webkit-reviews] review denied: [Bug 181805] Web Inspector: Layers tab should do away with popovers (if possible) : [Attachment 331695] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has denied Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 181805: Web Inspector: Layers tab should do away with popovers (if
possible)
https://bugs.webkit.org/show_bug.cgi?id=181805

Attachment 331695: Patch

https://bugs.webkit.org/attachment.cgi?id=331695&action=review




--- Comment #4 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 331695
  --> https://bugs.webkit.org/attachment.cgi?id=331695
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331695&action=review

r- for now. Overall this looks good. Most of my comments are style nits,
however I did notice that at narrow widths, the property values are not
vertically aligned as I'd expect. See the following screenshot.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:40
> +}

I think it would be cleaner to remove this class, as well as `display: none`
from .layer-info above, and instead toggle the global `hidden` style on the
div.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:43
> +    font-size: 1.1em;

We usually specify font-size in `px`. It's about 12px, which is a common size
used for title/heading type things across the UI.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:52
> +    line-height: 1.3em;

Convert to `px`.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:60
> +    -webkit-padding-start: 1em;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.css:64
> +    line-height: 1.3em;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:391
> +	   reasonsTitle.textContent = WI.UIString("Reasons for compositing:");

Remove the trailing colon. "Dimensions" doesn't have one. This panel is like a
floating details section, which doesn't include a colon after its title.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:403
> +	       this._layerInfoElement.classList.remove("showing");

See comment in Layers3DContentView.css.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:409
> +	   let visibleValue =
this._layerInfoElement.querySelector(".dimensions-visible");

Instead of querying, these elements can be member variables set by
_buildLayerInfoElement() at creation time.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:416
> +	       this._layerInfoElement.classList.add("showing");

this._layerInfoElement.classList.remove("hidden");

(see previous comments)

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:422
> +	   let reasonsList = this._layerInfoElement.querySelector(".reasons");

Use a member variable.