[webkit-reviews] review granted: [Bug 189485] Web Inspector: change WI.ColorWheel to use conic-gradient() : [Attachment 353115] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 189485: Web Inspector: change WI.ColorWheel to use conic-gradient()
https://bugs.webkit.org/show_bug.cgi?id=189485

Attachment 353115: Patch

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




--- Comment #11 from Brian Burg <bburg at apple.com> ---
Comment on attachment 353115
  --> https://bugs.webkit.org/attachment.cgi?id=353115
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:104
> +	       WI.ColorPicker._supportsConicGradient =
window.getComputedStyle(element).getPropertyValue(property).includes(conicGradi
ent);

You are clever.

> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:234
> +	       brightnessGradient = "linear-gradient(90deg, black, " + rawColor
+ ", white)";

Nit; Using template literals would be nice here

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:41
> +	   this._crosshair =
this._element.appendChild(document.createElement("div"));

Nit: _crosshairElement

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:157
> +	   var point =
window.webkitConvertPointFromPageToNode(this._gradientElement, new
WebKitPoint(event.pageX, event.pageY));

I've never heard of this before.

> Source/WebInspectorUI/UserInterface/Views/ColorWheel.js:169
> +	   if (center.distance(point) > radius) {

It would be nice to add a comment that this branch handles snapping to the edge
when the event is outside of the picker circle.