Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2157533003:
DevTools: Source color picker experiment. Make color swatches clickable (Closed)
|
Created:
4 years, 5 months ago by flandy Modified:
4 years, 5 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, blink-reviews-style_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Source color picker experiment. Make color swatches clickable
Improve the source color picker experiment by making the color swatches clickable.
On click, the swatches launch a spectrum to easily edit the stylesheet colors.
BUG=266326
R=lushnikov
Committed: https://crrev.com/1db875f82040068b874530dc594d74b17a434728
Cr-Commit-Position: refs/heads/master@{#406622}
Patch Set 1 #
Total comments: 8
Patch Set 2 : StylesPopoverHelper --> SwatchPopoverHelper in separate patches #
Total comments: 25
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed further comments #
Total comments: 2
Patch Set 5 : Consume event #Messages
Total messages: 20 (7 generated)
Description was changed from ========== DevTools: Source color picker experiment - Make color swatches clickable DevTools: Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. Also, move StylesPopoverHelper code and rename it so that it can be re-used in the sources panel. BUG=266326 R=lushnikov ========== to ========== DevTools: Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. Also, move StylesPopoverHelper code and rename it so that it can be re-used in the sources panel. BUG=266326 R=lushnikov ==========
Please take a look
first batch of comments https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js (left): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js:9: WebInspector.StylesPopoverHelper = function() Let's extract the StylesPopoverHelper in a few steps: -- Patch 1 -- 1. Rename it into WebInspector.SwatchPopoverHelper in this file 2. Unbind this component from StylesSidebarPane -- Patch 2 -- 1. Move WebInspector.SwatchPopoverHelper into a ui/SwatchPopoverHelper. (should be no changes to code itself) 2. Rename StylesPopoverHelper.js into ColorSwatchPopoverIcon.js https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js:67: this._scrollerElement = anchorElement.enclosingNodeOrSelfWithClass("style-panes-wrapper"); I would try inlining scroll/repositioning functionality to the ColorSwatchPopoverIcon/BezierIcon. Should not be a lot of code https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:297: this._swatch.removeClickListener(); this is hackish! We need to come up with something logical/meaningful https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:50: return this._colorValueElement.textContent; no need for the new method: colorSwatch.color().asString(colorSwatch.format())
Description was changed from ========== DevTools: Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. Also, move StylesPopoverHelper code and rename it so that it can be re-used in the sources panel. BUG=266326 R=lushnikov ========== to ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ==========
Description was changed from ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ========== to ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ==========
Please take another look https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js (left): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js:9: WebInspector.StylesPopoverHelper = function() On 2016/07/16 00:30:26, lushnikov wrote: > Let's extract the StylesPopoverHelper in a few steps: > > -- Patch 1 -- > 1. Rename it into WebInspector.SwatchPopoverHelper in this file > 2. Unbind this component from StylesSidebarPane > > -- Patch 2 -- > 1. Move WebInspector.SwatchPopoverHelper into a ui/SwatchPopoverHelper. (should > be no changes to code itself) > 2. Rename StylesPopoverHelper.js into ColorSwatchPopoverIcon.js Done. https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/StylesPopoverHelper.js:67: this._scrollerElement = anchorElement.enclosingNodeOrSelfWithClass("style-panes-wrapper"); On 2016/07/16 00:30:26, lushnikov wrote: > I would try inlining scroll/repositioning functionality to the > ColorSwatchPopoverIcon/BezierIcon. Should not be a lot of code Done. https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:297: this._swatch.removeClickListener(); On 2016/07/16 00:30:26, lushnikov wrote: > this is hackish! We need to come up with something logical/meaningful How about adding a function to ColorSwatch to set the click handler to something different than the default? https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2157533003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:50: return this._colorValueElement.textContent; On 2016/07/16 00:30:26, lushnikov wrote: > no need for the new method: > > colorSwatch.color().asString(colorSwatch.format()) Done.
https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:43: this._analyzeTextColors = true; We usually call this kind of flags like muteXXX, e.g.: this._muteColorProcessing = false; https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:186: this._spectrum.addEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this); When does this event fire? https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:215: delete this._spectrum; you should schedule updateColorSwatches here https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:235: if (Runtime.experiments.isEnabled("sourceColorPicker") && this._analyzeTextColors) nit: I'd check for the flag first https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:305: if (event.shiftKey) { I'm not convinced we need the shift-click in the source frame swatch - mb remove this functionality for now? https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:308: this._cssSourceFrame._replaceColorText(this._colorPosition, this._swatch.color().asString(this._swatch.format()) || "white"); where does "white" come from? https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:311: this._cssSourceFrame._currentSwatch = this; let's not reach to internal fields of cssSourceFrame from here - you already call it's delegate method _showSpectrum, it should do everything https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:312: this._cssSourceFrame._showSpectrum(this._swatch); I think it would be easier if you just pass the WI.CSSSourceFrame.ColorSwatch instance to the CSSSourceFrame: this._cssSourceFrame._showSpectrum(this); https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:320: getElement: function() element: ... (we use nouns for getters and verbs for setters) https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:328: getColorPosition: function() colorPosition: .. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:126: setClickHandler: function(clickHandler) let's just not handle clicks in the WI.ColorSwatch altogether and defer the logic to clients
Please take another look. Thanks! https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:43: this._analyzeTextColors = true; On 2016/07/19 21:12:16, lushnikov wrote: > We usually call this kind of flags like muteXXX, e.g.: > > this._muteColorProcessing = false; Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:186: this._spectrum.addEventListener(WebInspector.Spectrum.Events.SizeChanged, this._spectrumResized, this); On 2016/07/19 21:12:16, lushnikov wrote: > When does this event fire? This event fires when changing the color palette of the spectrum or adding a color to the custom color palette. The reposition deals with growing or shrinking the size of the palette. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:215: delete this._spectrum; On 2016/07/19 21:12:16, lushnikov wrote: > you should schedule updateColorSwatches here Why? We already change the color and position of the existing color swatch when the spectrum is changed. There should be no need to also update the swatches when the spectrum is hidden. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:235: if (Runtime.experiments.isEnabled("sourceColorPicker") && this._analyzeTextColors) On 2016/07/19 21:12:16, lushnikov wrote: > nit: I'd check for the flag first Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:305: if (event.shiftKey) { On 2016/07/19 21:12:16, lushnikov wrote: > I'm not convinced we need the shift-click in the source frame swatch - mb remove > this functionality for now? I think it would be nice to have, in order to mirror the functionality in the styles sidebar pane. Removed for now. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:308: this._cssSourceFrame._replaceColorText(this._colorPosition, this._swatch.color().asString(this._swatch.format()) || "white"); On 2016/07/19 21:12:16, lushnikov wrote: > where does "white" come from? color.asString() may return null|string according to closure compiler. cssSourceFrame._replaceColorText() expects a string, so this is to make the compiler happy. I can change the annotation for _replaceColorText() to use {?string} and introduce a check on that string Or I think it may make more sense to add the get colorText() method on WI.ColorSwatch and just call that. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:311: this._cssSourceFrame._currentSwatch = this; On 2016/07/19 21:12:16, lushnikov wrote: > let's not reach to internal fields of cssSourceFrame from here - you already > call it's delegate > method _showSpectrum, it should do everything Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:312: this._cssSourceFrame._showSpectrum(this._swatch); On 2016/07/19 21:12:16, lushnikov wrote: > I think it would be easier if you just pass the WI.CSSSourceFrame.ColorSwatch > instance > to the CSSSourceFrame: > > this._cssSourceFrame._showSpectrum(this); Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:320: getElement: function() On 2016/07/19 21:12:16, lushnikov wrote: > element: ... > > (we use nouns for getters and verbs for setters) Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:328: getColorPosition: function() On 2016/07/19 21:12:16, lushnikov wrote: > colorPosition: .. Done. https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:126: setClickHandler: function(clickHandler) On 2016/07/19 21:12:16, lushnikov wrote: > let's just not handle clicks in the WI.ColorSwatch altogether and defer the > logic to clients Removed for now since I have removed support for shift-click. Maybe I should defer the logic to clients in a separate patch?
https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:215: delete this._spectrum; On 2016/07/19 22:58:15, flandy wrote: > On 2016/07/19 21:12:16, lushnikov wrote: > > you should schedule updateColorSwatches here > > Why? We already change the color and position of the existing color swatch when > the spectrum is changed. There should be no need to also update the swatches > when the spectrum is hidden. Imagine you have multiple color swatches in the line and edit the first one https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ColorSwatch.js:126: setClickHandler: function(clickHandler) Sounds good for me https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:218: _replaceColorText: function(colorPosition, colorString) you can inline this now https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:282: WebInspector.CSSSourceFrame.ColorSwatch = function(cssSourceFrame, colorPosition) And it's clear that this class by itself doesn't have much value - let's inline the constructor in the _putColorSwatchesInline method: var swatch = WebInspector.ColorSwatch.create(); swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original)) swatch.iconElement().title = WebInspector.UIString("Open color picker."); swatch.iconElement().addEventListener("click", this._showSpectrum.bind(this, swatch, colorPosition), true); swatch.hideText(true); https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:288: this._swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original) || "white"); As you create color objects only with WebInspector.Color.parse, color object will always have original text. So, you can safely cast colorPosition.color.asString(WI.Color.Format.Original) to {!string}
Ptal https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:215: delete this._spectrum; On 2016/07/20 00:49:23, lushnikov wrote: > On 2016/07/19 22:58:15, flandy wrote: > > On 2016/07/19 21:12:16, lushnikov wrote: > > > you should schedule updateColorSwatches here > > > > Why? We already change the color and position of the existing color swatch > when > > the spectrum is changed. There should be no need to also update the swatches > > when the spectrum is hidden. > > Imagine you have multiple color swatches in the line and edit the first one Yes, good point! Done. https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:218: _replaceColorText: function(colorPosition, colorString) On 2016/07/20 00:49:23, lushnikov wrote: > you can inline this now Done. https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:282: WebInspector.CSSSourceFrame.ColorSwatch = function(cssSourceFrame, colorPosition) On 2016/07/20 00:49:23, lushnikov wrote: > And it's clear that this class by itself doesn't have much value - let's inline > the constructor in the _putColorSwatchesInline method: > > var swatch = WebInspector.ColorSwatch.create(); > swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original)) > swatch.iconElement().title = WebInspector.UIString("Open color picker."); > swatch.iconElement().addEventListener("click", this._showSpectrum.bind(this, > swatch, colorPosition), true); > swatch.hideText(true); Done. https://codereview.chromium.org/2157533003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:288: this._swatch.setColorText(colorPosition.color.asString(WebInspector.Color.Format.Original) || "white"); On 2016/07/20 00:49:23, lushnikov wrote: > As you create color objects only with WebInspector.Color.parse, color object > will always > have original text. > > So, you can safely cast colorPosition.color.asString(WI.Color.Format.Original) > to {!string} The compiler no longer complains about this after inlining the constructor.
lgtm! https://codereview.chromium.org/2157533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:180: _showSpectrum: function(swatch, colorPosition) don't you want to consume event here?
https://codereview.chromium.org/2157533003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js (right): https://codereview.chromium.org/2157533003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CSSSourceFrame.js:180: _showSpectrum: function(swatch, colorPosition) On 2016/07/20 02:45:45, lushnikov wrote: > don't you want to consume event here? Done.
The CQ bit was checked by flandy@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2157533003/#ps80001 (title: "Consume event")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ========== to ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov ========== to ========== DevTools: Source color picker experiment. Make color swatches clickable Improve the source color picker experiment by making the color swatches clickable. On click, the swatches launch a spectrum to easily edit the stylesheet colors. BUG=266326 R=lushnikov Committed: https://crrev.com/1db875f82040068b874530dc594d74b17a434728 Cr-Commit-Position: refs/heads/master@{#406622} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1db875f82040068b874530dc594d74b17a434728 Cr-Commit-Position: refs/heads/master@{#406622} |
