[webkit-reviews] review granted: [Bug 219996] Web Inspector: Font Details sidebar - Improve visibility of values by emphasizing them/de-emphasizing range information : [Attachment 417183] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 219996: Web Inspector: Font Details sidebar - Improve visibility of values
by emphasizing them/de-emphasizing range information
https://bugs.webkit.org/show_bug.cgi?id=219996

Attachment 417183: Patch v1.0

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




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

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

r=me, with a few fixes/questions :)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:26
> +.font > .details-section > .content > .group > .row.simple > .value
.secondary {

Starting with `.font` is super generic.  Please change it to `.sidebar >
.panel.details.font` to be more specific (and match other details sidebar
panels).

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.css:28
> +    word-break: normal;

Is this necessary?  IIRC the default value of `word-break` is `normal`.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:180
> +	   let secondaryElement = document.createElement("span");

NIT: you can combine this and 183 to be one line
```
    let secondaryElement =
valueElement.appendChild(document.createElement("span"));
```

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:182
> +	   secondaryElement.textContent = WI.UIString(" (Range: %d-%d)", "
(Range: %d-%d) @ Font Details Sidebar", "A range for a single variation axis of
a font.").format(minimumValue, maximumValue);

Do we no longer care about the `defaultValue`?