[webkit-reviews] review granted: [Bug 192578] Web Inspector: Computed: make UI more usable when the panel is narrow : [Attachment 357657] 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 192578: Web Inspector: Computed: make UI more usable when the panel is
narrow
https://bugs.webkit.org/show_bug.cgi?id=192578

Attachment 357657: Patch

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




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

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

r=me, I think we might actually benefit from having a little padding/offset
between the actual computed value and its override chain.  When looking at
semi-complex properties (e.g. gradients) it can sometimes be confusing where
one value ends and another value begins.  Adding an inset (like the default
list styling) or some vertical space before each item in the override chain
might help distinguish values from each other.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:55
> +    vertical-align: top;

Is this needed?  When I tried removing it, it looked like it worked the same.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:84
> +    vertical-align: top;

This is already set on the non-`.expanded` rule.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:94
> +    content: "â¢";

That's unfortunate :(  You should use the escaped unicode sequence ("\2022") so
it's nice and pretty inside diffs.

Also, adding this causes the text after it to become ever-so-slightly indented
when compared to the actual computed value.  You might want to offset the text
slightly to ensure that it aligns horizontally.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:96
> +    width: 0.8em;

Ditto (>55).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:97
> +    margin-left: -0.8em;

Is there a way to get the actual width of the disclosure triangle and divide it
by two?  ComputedStyleSection.css has `calc(var(--disclosure-button-size) +
6px)`, so you could rework that into a variable and use
`calc(var(--disclosure-area-width) / 2)` as your value instead of something
hardcoded.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:151
>  .computed-style-section .property-trace-item .property .name + span {

Maybe we should give a class to these <span>s, like "brace"?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:152
> +    display: none;

Merge this rule with the following one so there's no repetition.