Comment on attachment 8989868 [details] Bug 1472566 - Add a color picker to the flexbox listings to change the flexbox highlighter colour. https://reviewboard.mozilla.org/r/254856/#review262756 Looks good, I would like to see some mochitests covering this feature, similar to the ones we have for the grid inspector. Feel free to log it as a follow up bug or to change the reviewer since I am not accepting other review requests this week. ::: devtools/client/inspector/flexbox/components/FlexboxItem.js:48 (Diff revision 1) > + flexbox, > + getSwatchColorPickerTooltip, > + onSetFlexboxOverlayColor, > + } = this.props; > + > + const swatchEl = findDOMNode(this).querySelector(".flexbox-color-swatch"); We should be able to use refs instead of this, but refs APIs are changing with the recent versions of React, so fine with keeping querySelector() ::: devtools/client/inspector/flexbox/components/FlexboxItem.js:148 (Diff revision 1) > + ), > + // The SwatchColorPicker relies on the nextSibling of the swatch element to apply > + // the selected color. This is why we use a span in display: none for now. > + // Ideally we should modify the SwatchColorPickerTooltip to bypass this requirement. > + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1341578 > + dom.span({ className: "flexbox-color-value" }, color) Seems like there is a lot of duplication between this and GridItem.js. Would it make sense to log a follow up to introduce a shared component here? ::: devtools/client/inspector/flexbox/flexbox.js:330 (Diff revision 1) > + const currentUrl = this.inspector.target.url; > + // Get the hostname, if there is no hostname, fall back on protocol > + // ex: `data:` uri, and `about:` pages > + const hostname = parseURL(currentUrl).hostname || parseURL(currentUrl).protocol; > + const customColors = await asyncStorage.getItem("flexboxInspectorHostColors") || {}; Can we extract this duplicated logic to a single method? ::: devtools/client/inspector/grids/grid-inspector.js:86 (Diff revision 1) > - return this._swatchColorPickerTooltip; > - } > - > /** > * Initializes the grid inspector by fetching the LayoutFront from the walker, loading > * the highlighter settings and initalizing the SwatchColorPicker instance. update this comment to remove mentions of SwatchColorPicker ::: devtools/client/inspector/layout/layout.js:65 (Diff revision 1) > onToggleShowGridLineNumbers, > onToggleShowInfiniteLines, > } = this.gridInspector.getComponentProps(); > > const layoutApp = LayoutApp({ > - getSwatchColorPickerTooltip, > + getSwatchColorPickerTooltip: this.getSwatchColorPickerTooltip, maybe we could simply have getSwatchColorPickerTooltip: () => this.swatchColorPickerTooltip and drop the bind() and the extra method? ::: devtools/client/inspector/layout/layout.js:102 (Diff revision 1) > > /** > * Destruction function called when the inspector is destroyed. Cleans up references. > */ > destroy() { > + // The color picker may not be ready as `init` function is async, This comment feels wrong here, because init() is not async. Can you reformulate or drop the comment?

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/727a835af2ff Add a color picker to the flexbox listings to change the flexbox highlighter colour. r=jdescottes

Backed out for failing devtools at devtools/client/inspector/grids/test/browser_grids_color-in-rules-grid-toggle.js Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=727a835af2ff6a6aa1f099d73f27ed9cd863b82f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188627956&repo=mozilla-inbound&lineNumber=1651 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/06aa3087bced7348e14059dea0f676ec84faf8ca

Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/74bf9fea4dee Add a color picker to the flexbox listings to change the flexbox highlighter colour. r=jdescottes