[webkit-reviews] review granted: [Bug 191984] Web Inspector: Computed panel: allow to expand properties to show list of overridden values : [Attachment 355767] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 191984: Web Inspector: Computed panel: allow to expand properties to show
list of overridden values
https://bugs.webkit.org/show_bug.cgi?id=191984

Attachment 355767: Patch

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




--- Comment #11 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 355767
  --> https://bugs.webkit.org/attachment.cgi?id=355767
Patch

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

r=me, with some expected followups:
 - Add tabbing support
 - Discuss/Finalize styling
 - Move Variables section to use this code as well

Looks great :)

> Source/WebInspectorUI/ChangeLog:97
> +	   Replace `_renderOrigin` with WI.StyleOrigin so it could be used by
WI.ComputedStyleSection. 

WI.StyleOriginView

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:47
> +    background-color: hsl(0, 0%, 97%);

I personally think that we should keep the collapsed color be the standard
`--background-color`, and having this intermediate color is somewhat confusing,
but we could discuss/fix that later.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:51
> +    font: 12px -webkit-system-font, sans-serif;

The `font-size` of the values in the "Variables" and "Box Model" sections is
`11px` (this is our standard `font-size` as well).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:171
> +		   if (properties === undefined) {

Style: use `!` instead of `=== undefined` (we aren't that explicit unless
`null` or `false` are valid values), or even check with `Array.isArray`.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:235
> +		   if (ownerRule)

We should have some text for inline styles as well.

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:59
> +	   const shouldExpand = !this._expandedSetting.value;

Style: use `let` not `const` (I reserve `const` for values that shouldn't
change over the life of the program or even between runs of the program).

> Source/WebInspectorUI/UserInterface/Views/StyleOriginView.js:28
> +    constructor(style)

I'd reiterate my comment in attachment 355686, in that you should rework this
class to have an `update` function that takes a `WI.CSSStyleDeclaration` and
regenerates its DOM so that you don't need to call `replaceWith`.