[Webkit-unassigned] [Bug 117919] Web Inspector: Display color picker in popover on color swatch click

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117919





--- Comment #5 from Timothy Hatcher <timothy at apple.com>  2013-06-24 11:12:50 PST ---
(From update of attachment 205271)
View in context: https://bugs.webkit.org/attachment.cgi?id=205271&action=review

Great job! Just some minor issues that need addressed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:38
> +    -webkit-user-select: none;

No need for this. We have it set globally.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:113
> +.colorpicker-dragger, .colorpicker-slider {
> +    -webkit-user-select: none;
> +}

Not needed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:115
> +.colorpicker-sat {

You should spell out saturation.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.css:119
> +.colorpicker-val {

value? luminance? Whatever it is, spell it out.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:39
> +    // TODO: ported from Spectrum.js Not sure if this stays?
> +    //this._element.tabIndex = 0;

I don't think it is needed.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:49
> +    this._dragHelperElement = this._draggerElement
> +    .createChild("div", "colorpicker-sat colorpicker-fill-parent")
> +    .createChild("div", "colorpicker-val colorpicker-fill-parent")
> +    .createChild("div", "colorpicker-dragger");

I'm not sold on using the createChild helper in a chain like this. At the very least, the last three lines should be indented.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:57
> +    //alphaLabel.textContent = WebInspector.UIString(
> +    alphaLabel.textContent = "\u03B1:";

Use "Opacity:" and put the string in localizableStrings.js in alphabetical order.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:76
> +    function makeDraggable(element, onmove, onstart, onstop) {
> +        var doc = document;

{ should be on a new line.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:90
> +            if (dragging) {

This should return early to prevent indenting the whole function body.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:101
> +            var rightClick = e.which ? (e.which === 3) : (e.button === 2);

No need for ().

We prefer:
if (event.button !== 0 || event.ctrlKey)
    return;

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:105
> +            if (!rightClick && !dragging) {
> +
> +                if (onstart)

Remove empty line here.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:138
> +            if (dragging) {
> +                doc.removeEventListener("selectstart", consume, false);
> +                doc.removeEventListener("dragstart", consume, false);
> +                doc.removeEventListener("mousemove", move, false);
> +                doc.removeEventListener("mouseup", stop, false);
> +
> +                if (onstop)
> +                    onstop(element, e);
> +            }
> +
> +            dragging = false;

This should return early to prevent indenting the whole function body. No need for dragging = false then either.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:180
> +    }
> +
> +};

Remove empty line.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:246
> +            if (format === cf.RGBA)
> +                format = cf.RGB;
> +            else if (format === cf.HSLA)
> +                format = cf.HSL;
> +            // Don't output ShortHex
> +            else if (format === cf.ShortHEX) 
> +                format = cf.HEX;
> +        } else {
> +            // Everything except HSL(A) should be returned as RGBA if transparency is involved.
> +            if (format === cf.HSL || format === cf.HSLA)
> +                format = cf.HSLA;
> +            else
> +                format = cf.RGBA;

Use the full WebInspector.Color.Format... constant in all these places. Shortening them hinders find and replace for renaming in the future.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:290
> +                        Math.min(this.dragWidth - this._dragHelperElementHeight, dragX - this._dragHelperElementHeight));

Only indent 4 spaces. Don't line up.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:292
> +                        Math.min(this.dragHeight - this._dragHelperElementHeight, dragY - this._dragHelperElementHeight));

Ditto.

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:324
> +WebInspector.CSSColorPicker.hsvaToRGBA = function(h, s, v, a)

Should this be moved to WebInspector.Color as a helper function?

> Source/WebInspectorUI/UserInterface/CSSColorPicker.js:358
> +WebInspector.CSSColorPicker.rgbaToHSVA = function(r, g, b, a)

Ditto?

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:431
> -                    swatchElement.title = WebInspector.UIString("Click to toggle color format");
> +                    swatchElement.title = WebInspector.UIString("Click to open a color picker. Shift-click to toggle color format.");

Need to update localizableStrings.js.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:672
> +            function _update()

A more descriptive name without the underscore would be better.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:688
> +            }
> +            this._codeMirror.operation(_update.bind(this))

Empty line between these two lines.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:708
> +        } else {
> +
> +            if (this._colorPickerPopover) {

Remove empty line.

Also I think there is a bug if the popover dismisses by clicking away (more common than clicking the swatch again). Only if the user clicks the swatch again will this._colorPickerPopover be deleted. Clicking another swatch would not show the popover on first click. You shouldn't need to hold onto the Popover, but clicking the same swatch again should dismiss the current popover and not show another one again. Perhaps Popover.js's handleEvent should preventDefault and/or stopPropagation when it calls dismiss? Then this code will not be called.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:716
> +            var colorPicker = new WebInspector.CSSColorPicker();

Omit the () when there are no params.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:724
> +            this._colorPickerPopover.content = colorPicker.element;
> +            var bounds = WebInspector.Rect.rectFromClientRect(swatch.getBoundingClientRect());

Empty line between these lines would help readability.

> Source/WebInspectorUI/UserInterface/CSSStyleDeclarationTextEditor.js:726
> +            this._colorPickerPopover.present(bounds, [WebInspector.RectEdge.MIN_X]);
> +            colorPicker.shown();

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.