[webkit-reviews] review granted: [Bug 203169] Web Inspector: Replace color wheel with square HSB color picker : [Attachment 381624] 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 203169: Web Inspector: Replace color wheel with square HSB color picker
https://bugs.webkit.org/show_bug.cgi?id=203169

Attachment 381624: Patch

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




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

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:34
> +	   this._hueSlider = new WI.Slider;

It's odd that the `0` of the hue slider is at the bottom.  I'd almost expect it
to be at the top.  Could we flip it vertically?

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:-103
> -	   this._colorWheel.brightness = brightness;

I don't think `set brightness` is used anywhere anymore.  We should remove it.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:119
> +	   return this._colorSquare;

Style: I'd make this (and `get element`) into one-line getters at the top of
the Public section

>
Source/WebInspectorUI/UserInterface/Views/ColorSquare.cssSource/WebInspectorUI/
UserInterface/Views/ColorWheel.css:53
> +    border: 0.5px solid black;

We should only do this if we are on a @2x device.  Please make this `1px` and
add a `@media (-webkit-min-device-pixel-ratio: 2)` query for `border-width:
0.5px;`.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:53
> +	   this._updateBaseColor();

This is already called by `set dimension`, so we can remove this.

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:101
> +	   y = -(y - 1) * this._dimension;

NIT: I'd personally use `-1 * (` instead of `-(` so it's less likely to be
missed.