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

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

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 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 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 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.

Applies changes to code as requested by review.

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.

Fix indentation as requested in recent review.

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 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 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 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.