Closed Bug 1332086 Opened 4 years ago Closed 2 years ago

Rearrange the existing color picker to the new design

Categories

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

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1478152

People

(Reporter: gl, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 6 obsolete files)

This bug is to rearrange the current existing color picker so it aligns with the new design. This bug is reserved for UCOSP students.
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
Attached patch Bug1332086.patch (obsolete) — Splinter Review
Help on calculating the hue with the hue slider being swapped from top-to-bottom to left-to-right
Attachment #8828992 - Flags: feedback?(gl)
Attachment #8828992 - Flags: feedback?(gl)
Attached patch Bug1332086.patch (obsolete) — Splinter Review
Attachment #8829050 - Flags: review?(gl)
Attachment #8828992 - Attachment is obsolete: true
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()
Attachment #8828992 - Attachment is obsolete: false
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
Attachment #8829050 - Flags: review?(gl)
Attached patch Bug1332086.patch (obsolete) — Splinter Review
Attachment #8829134 - Flags: review?(gl)
Attached patch Bug1332086v4.patch (obsolete) — Splinter Review
Attachment #8828992 - Attachment is obsolete: true
Attachment #8829050 - Attachment is obsolete: true
Attachment #8829134 - Attachment is obsolete: true
Attachment #8829134 - Flags: review?(gl)
Attachment #8829165 - Flags: review?(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.
Attachment #8829165 - Flags: review?(gl)
Attachment #8829165 - Attachment is obsolete: true
Attachment #8831009 - Flags: review?(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.
Attachment #8831009 - Flags: review?(gl)
Attached patch Bug1332086v5.patch (obsolete) — Splinter Review
Applies changes to code as requested by review.
Attachment #8831009 - Attachment is obsolete: true
Attachment #8834121 - Flags: review?(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.
Attachment #8834121 - Flags: review?(gl) → review+
Fix indentation as requested in recent review.
Attachment #8834121 - Attachment is obsolete: true
Attachment #8835262 - Flags: review?(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.
Attachment #8835262 - Flags: review?(gl) → review+
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.
Attachment #8831012 - Flags: review?(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
Attachment #8863989 - Flags: review?(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.
Attachment #8863989 - Flags: review?(gl)
Attachment #8863989 - Flags: review?(gl)
Product: Firefox → DevTools
Assignee: tigleym → mtigley

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.

Assignee: mtigley → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1478152
You need to log in before you can comment on or make changes to this bug.