[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
bugzilla-daemon at webkit.org
Fri Jan 19 12:54:54 PST 2018
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.
More information about the webkit-reviews
mailing list