1332085 - Add Color Name field to Color Widget

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Add the Named Color component to the Color Widget. This bug is reserved for UCOSP students.

Comment 1

[Sheldon Roddick [:sroddick]]

Attachment: Bug1332085.patch v1

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8828993 [details] [diff] [review]
Bug1332085.patch v1

Review of attachment 8828993 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/locales/en-US/colorwidget.properties
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE These strings are used in the CSS Color Editor Widget
> +# which can be found in a tooltip that appears in the Rule View when clicking

s/Rule/Rules

@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE These strings are used in the CSS Color Editor Widget
> +# which can be found in a tooltip that appears in the Rule View when clicking
> +# on a color swatch displayed next to CSS declarations like 'color: #FFF'

Add a period to the end of the sentence.

@@ +6,5 @@
> +# which can be found in a tooltip that appears in the Rule View when clicking
> +# on a color swatch displayed next to CSS declarations like 'color: #FFF'
> +
> +# LOCALIZATION NOTE (colorNameLabel):
> +# This string is displayed to indicate the field in which the currently

I think we should move the string so it starts right after the ":", and I think we can simplify the string to read:
The label for the current color widget's color name field.

::: devtools/client/shared/test/browser_color-widget-01.js
@@ +117,5 @@
> +function testChangingColorShouldUpdateColorNameDisplay(container) {
> +  let s = new ColorWidget(container, [255, 255, 255, 1]);
> +  s.show();
> +  s.updateUI();
> +  ok(s.colorName.textContent === "white", "Color Name should be white, ColorWidget name change failed");

We should be using is so we know the expected value.

is(s.colorName.textContent, "white", "The Color Name is White");

The is() signature is is(actual, expected, message).

::: devtools/client/shared/widgets/ColorWidget.js
@@ +14,1 @@
>  const XHTML_NS = "http://www.w3.org/1999/xhtml";

Move XHTML_NS after the L10N declaration with a new line to separate it.

@@ +344,4 @@
>    },
>  
>    destroy: function () {
>      this.element.removeEventListener("click", this.onElementClick);

We need to set this.colorName to null in destroy()

::: devtools/shared/css/color.js
@@ +175,3 @@
>        return this.hex;
>      }
> +    let {r, g, b} = tuple;

Add a new line after if statement block.

@@ +548,5 @@
>          cssRGBMap[key] = name;
>        }
>      }
>    }
> +  return cssRGBMap[JSON.stringify([r, g, b, 1])] || "";

We should update the @return {String} to indicate the name of the color or an empty string if the color does not have a name. We also need to update the JSDoc to say it doesn't throw but returns an empty string as well.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a0b0580c23e92ca6effe80a44a525e3d237389

Comment 4

[Sheldon Roddick [:sroddick]]

Attachment: Bug1332085.patch v2

Comment 5

[Sheldon Roddick [:sroddick]]

Attachment: Bug1332085.patch v3

Comment 6

[Sheldon Roddick [:sroddick]]

Attachment: Bug1332085.patch v4

Comment 7

[Sheldon Roddick [:sroddick]]

Attachment: Bug1332085.patch v5

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e329450e4d25
Add Color Name field to Color Widget. r=gl

Comment 9

[Phil Ringnalda (:philor)]

Sorry, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/26366617b865 for pretty frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=70968730&repo=mozilla-inbound

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61699325a7ad9d9335a91bcc8aea026a0b47bce7&selectedJob=70973130

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98530de82f5c
Add Color Name field to Color Widget. r=gl

Comment 12

[Phil Ringnalda (:philor)]

Sorry, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b03d23d20564 for pretty frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=71032748&repo=mozilla-inbound

Comment 13

[Micah Tigley [:micah]]

Attachment: Bug 1332085 - Add Color Name Field to Color Widget.

Review commit: https://reviewboard.mozilla.org/r/137354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137354/

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8865729 [details]
Bug 1332085 - Add Color Name Field to Color Widget.

https://reviewboard.mozilla.org/r/137354/#review140882

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40129d19820
Add Color Name Field to Color Widget. r=gl

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c40129d19820