1332086 - Rearrange the existing color picker to the new design

Status: RESOLVED DUPLICATE of bug 1478152

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

Description

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

This bug is to rearrange the current existing color picker so it aligns with the new design. This bug is reserved for UCOSP students.

Comment 1

[Micah Tigley [:micah]]

Attachment: Bug1332086.patch

Help on calculating the hue with the hue slider being swapped from top-to-bottom to left-to-right

Comment 2

[Micah Tigley [:micah]]

Attachment: Bug1332086.patch

Comment 3

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

Comment on attachment 8829050 [details] [diff] [review]
Bug1332086.patch

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

The 2 commits you currently need to be squashed to form one patch. Otherwise, looks pretty good so far.

::: devtools/client/shared/widgets/ColorWidget.js
@@ +282,5 @@
>  
>    onChange: function () {
>      this.emit("changed", this.rgb, this.rgbCssString);
> +    this.colorPreview.style.backgroundColor =
> +    `rgb(${this.rgb[0]}, ${this.rgb[1]}, ${this.rgb[2]}`;

We can use this.rgbCssString here. We should also move this backgroundColor setter into updateUI()

Comment 4

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

Comment on attachment 8828992 [details] [diff] [review]
Bug1332086.patch

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

::: devtools/client/shared/widgets/ColorWidget.js
@@ +309,5 @@
>      this.dragHelper.style.top = dragY + "px";
>      this.dragHelper.style.left = dragX + "px";
>  
>      // Placing the hue slider
> +    let hueSliderX = (this.hsv[0] * this.hueSliderWidth) -

h is still equivalent to this.hsv[0]. I would continue to use h here.

@@ +342,5 @@
>      this.element.removeEventListener("click", this.onElementClick);
>  
>      this.parentEl.removeChild(this.element);
>  
> +    this.hueSlider = this.hueInnerSlider = this.hueSliderHelper = null;

Move this to be after this.alphaSlider line

Comment 5

[Micah Tigley [:micah]]

Attachment: Bug1332086.patch

Comment 6

[Micah Tigley [:micah]]

Attachment: Bug1332086v4.patch

Comment 7

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

Comment on attachment 8829165 [details] [diff] [review]
Bug1332086v4.patch

Clearing review. We need to remove the patch file that was accidentally included in the actual patch file, but otherwise please breakdown this patch into 2 parts - 1 for the feature, 2 for the test.

Comment 8

[Micah Tigley [:micah]]

Attachment: This patch contains the feature itself.

Comment 9

[Micah Tigley [:micah]]

Attachment: Contains the unit tests for this feature

Comment 10

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

Comment on attachment 8831009 [details] [diff] [review]
This patch contains the feature itself.

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

::: devtools/client/shared/widgets/ColorWidget.js
@@ +56,3 @@
>        </div>
>      </div>
> +    <div class="bottom-container">

Since the top container was called colorwidget-top, we should probably rename this to colorwidget-bottom.

@@ +56,4 @@
>        </div>
>      </div>
> +    <div class="bottom-container">
> +      <div class="color-preview"></div>

s/color/colorwidget-color-preview or colorwidget-preview

@@ +57,5 @@
>      </div>
> +    <div class="bottom-container">
> +      <div class="color-preview"></div>
> +      <button id="eyedropper-button" class="devtools-button"></button>
> +      <div class="slider-container">

Prefix this by colorwidget-. This is especially important when we eventually move into inline editors because all the css stylesheets will be loaded inside of inspector.xhtml, and the easiest way to differentiate the various element selctors is by prefixing the widget/component it belongs to.

@@ +87,5 @@
>    this.alphaSliderHelper = this.element.querySelector(".colorwidget-alpha-handle");
>    ColorWidget.draggable(this.alphaSliderInner, this.onAlphaSliderMove.bind(this));
>  
> +  this.hueSlider = this.element.querySelector(".colorwidget-hue");
> +  this.hueInnerSlider = this.element.querySelector(".colorwidget-hue-inner");

s/this.hueInnerSlide/this.hueSliderInner for consistency.

@@ +243,5 @@
>  
>    show: function () {
>      this.element.classList.add("colorwidget-show");
>  
> +    this.hueSliderWidth = this.hueInnerSlider.offsetWidth;

Move this below this.alphaSliderHelperWidth

@@ +260,5 @@
>    onElementClick: function (e) {
>      e.stopPropagation();
>    },
>  
> +  onHueSliderMove: function (dragX, dragY) {

Move this function below onAlphaSliderMove

@@ +312,5 @@
>      this.dragHelper.style.top = dragY + "px";
>      this.dragHelper.style.left = dragX + "px";
>  
>      // Placing the hue slider
> +    let hueSliderX = (h * this.hueSliderWidth) -

Move this below alphaSliderX. All of this refactoring is just to keep things somewhat alphabetically ordered.

@@ +313,5 @@
>      this.dragHelper.style.left = dragX + "px";
>  
>      // Placing the hue slider
> +    let hueSliderX = (h * this.hueSliderWidth) -
> +      (this.hueSliderHelperWidth / 2);

I don't think this needs to be in a new line since it is <90 char.

@@ +332,5 @@
>      let flatColor = "rgb(" + rgbNoSatVal[0] + ", " + rgbNoSatVal[1] + ", " +
>        rgbNoSatVal[2] + ")";
>  
>      this.dragger.style.backgroundColor = flatColor;
> +    this.colorPreview.style.backgroundColor = this.rgbCssString;

Move this above this.dragger to keep this alphabetically ordered.

::: devtools/client/shared/widgets/color-widget.css
@@ +91,5 @@
>    right: 0;
>    bottom: 0;
>  }
>  
>  .colorwidget-dragger, .colorwidget-slider {

Remove .colorwidget-slider since that longer exists.

@@ +95,5 @@
>  .colorwidget-dragger, .colorwidget-slider {
>    -moz-user-select: none;
>  }
>  
> +.colorwidget-alpha, .colorwidget-hue {

Format this to be 
.colorwidget-alpha,
.colorwidget-hue {

@@ +101,5 @@
>    height: 8px;
>    margin-top: 3px;
>  }
>  
> +.colorwidget-alpha-inner, .colorwidget-hue-inner {

Same as above

@@ +106,4 @@
>    height: 100%;
>  }
>  
> +.colorwidget-alpha-handle, .colorwidget-hue-handle {

Same as above

@@ +137,5 @@
>    border: 1px solid white;
>    box-shadow: 0 0 2px rgba(0,0,0,.6);
>  }
>  
>  .colorwidget-slider {

We don't actually have a colorwidget-slider anymore since this was the hue slider. So, this can be removed.

Comment 11

[Micah Tigley [:micah]]

Attachment: Bug1332086v5.patch

Applies changes to code as requested by review.

Comment 12

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

Comment on attachment 8834121 [details] [diff] [review]
Bug1332086v5.patch

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

::: devtools/client/shared/widgets/ColorWidget.js
@@ +328,5 @@
>      let rgb = this.rgb;
>      let rgbNoSatVal = this.rgbNoSatVal;
>  
>      let flatColor = "rgb(" + rgbNoSatVal[0] + ", " + rgbNoSatVal[1] + ", " +
> +    rgbNoSatVal[2] + ")";

We should keep the indentation.

Comment 13

[Micah Tigley [:micah]]

Attachment: Bug1332086v6.patch

Fix indentation as requested in recent review.

Comment 14

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

Comment on attachment 8835262 [details] [diff] [review]
Bug1332086v6.patch

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

::: devtools/client/shared/widgets/color-widget.css
@@ +91,5 @@
>    right: 0;
>    bottom: 0;
>  }
>  
> +.colorwidget-dragger, {

I think you intended to put the .colorwidget-slider on a new line, but not outright remove it? Otherwise, just undo the changes for this line.

Comment 15

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

Comment on attachment 8831012 [details] [diff] [review]
Contains the unit tests for this feature

Clearing review since we are gonna wait for the inline widget to land before rearranging this.

Comment 16

[Micah Tigley [:micah]]

Attachment: Bug 1332086 - Rearrange the existing color picker to the new design.

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

Comment 17

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

Comment on attachment 8863989 [details]
Bug 1332086 - Rearrange the existing color picker to the new design.

https://reviewboard.mozilla.org/r/135710/#review140916

Clearing the r? since this will need to be rebased

Comment 18

[Micah Tigley [:micah]]

Comment on attachment 8863989 [details]
Bug 1332086 - Rearrange the existing color picker to the new design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135710/diff/1-2/

Comment 19

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

Comment on attachment 8863989 [details]
Bug 1332086 - Rearrange the existing color picker to the new design.

https://reviewboard.mozilla.org/r/135710/#review175108

Needs a rebase.

Comment 20

[Micah Tigley [:micah]]

Comment on attachment 8863989 [details]
Bug 1332086 - Rearrange the existing color picker to the new design.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135710/diff/2-3/

Comment 21

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.