[webkit-reviews] review granted: [Bug 233054] Web Inspector: Add a swatch for align-items and align-self : [Attachment 446185] Patch

[bugzilla-daemon at webkit.org]

Patrick Angle <pangle at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 233054: Web Inspector: Add a swatch for align-items and align-self
https://bugs.webkit.org/show_bug.cgi?id=233054

Attachment 446185: Patch

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




--- Comment #16 from Patrick Angle <pangle at apple.com> ---
Comment on attachment 446185
  --> https://bugs.webkit.org/attachment.cgi?id=446185
Patch

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

r=me with a few nits.

While I'm thinking about it, please open bug(s) for the visual adjustments we
discussed this morning (button size/margin/padding, cleaning up the tiny icons,
etc.)

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:41
> +	   let glyphs =
Object.entries(WI.AlignmentEditor._glyphsForProperty(propertyName));
> +	   for (let [value, path] of glyphs) {

Nit: I'd keep `glyphs` inlined.

> Source/WebInspectorUI/UserInterface/Views/AlignmentEditor.js:64
> +	   return glyphs[value] || WI.AlignmentEditor.UnknownValueGlyph;

Just in case, can we `return glyphs?.[value] || ...` so that when failing the
assertion we still return a sensical value instead of raising another exception
here?