1472566 - Add a color picker to the flexbox listings to change the flexbox highlighter colour

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: Bug 1472566 - Add a color picker to the flexbox listings to change the flexbox highlighter colour.

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

Comment 2

[Julian Descottes [:jdescottes]]

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?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

Comment 4

[Andreea Pavel [:apavel]]

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

Comment 5

[Pulsebot]

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

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/74bf9fea4dee