1484483 - Show the flex container properties in the Flexbox panel

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: Part 1: Refactor the flex container listing out of the Flexbox Component into FlexContainerList. [1.0]

Comment 2

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

Attachment: Bug 1484483 - Part 2: Remove the computed style properties of the flex container from the FlexboxActor. [1.0]

Comment 3

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

Attachment: Part 2: Return the computed style properties of the flex container from the FlexboxActor. [2.0]

Comment 4

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

Attachment: Part 3: Show the list of flex container properites in the Flexbox panel. [1.0]

Comment 5

[Razvan Caliman [:rcaliman]]

Comment on attachment 9002313 [details] [diff] [review]
Part 1: Refactor the flex container listing out of the Flexbox Component into FlexContainerList. [1.0]

Review of attachment 9002313 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/components/FlexContainerItem.js
@@ +21,3 @@
>    static get propTypes() {
>      return {
>        flexbox: PropTypes.shape(Types.flexbox).isRequired,

Question:

Only `flexbox.color` and `flexbox.nodeFront` seem to be used at this level. Do you think it's better to pass these in as props instead of the `flexbox` reference? Or do you foresee needing more access to what's inside `flexbox`?

::: devtools/client/inspector/flexbox/components/Flexbox.js
@@ +17,5 @@
>  
>  class Flexbox extends PureComponent {
>    static get propTypes() {
>      return {
>        flexbox: PropTypes.shape(Types.flexbox).isRequired,

I don't understand where `flexbox` is coming from. 
As I read the code, these props seem to come from the propos of LayoutApp.js:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/components/LayoutApp.js#84

Yet there is no `flexbox` prop there either in the propTypes or in the component instantiation which happens in layout.js

The propTypes here specify that `flexbox` it's required and I don't get any errors in the console. So it does comes from somewhere, but I'm puzzled.

[Later edit, but I'll keep the above comment because I think it's still valid]

I figured out that `flexbox` here is a reference to the Redux store instance. But that's very opaque. Not a blocker for this review, but we should really make it more explicit that it's a Redux store and not a component prop. As it is right now, it is very confusing. Perhaps a comment and/or a better name.

@@ +34,1 @@
>        getSwatchColorPickerTooltip,

A `getSwatchColorPickerTooltip` is exported by `getComponentProps()` in flexbox.js:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/flexbox/flexbox.js#96

However, that seems to be a dead reference that is never used:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/layout.js#49-52

The `getSwatchColorPickerTooltip` here is actually coming via LayoutApp.js from layout.js:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/layout.js#65

If the reference in flexbox.js is indeed not used, please remove it from there to avoid confusion.

@@ +35,5 @@
>        setSelectedNode,
>        onHideBoxModelHighlighter,
>        onSetFlexboxOverlayColor,
>        onShowBoxModelHighlighterForNode,
>        onToggleFlexboxHighlighter,

`onToggleFlexboxHighlighter` doesn't seem to be defined in LayoutApp's propTypes:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/components/LayoutApp.js#36-54

It does get passed in as a prop when creating an instance of LayoutApp in 
https://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/layout.js#79

Please update the propTypes in LayoutApp.js so it's possible to trace the origin of this reference.

@@ +41,5 @@
>  
> +    if (!flexbox.actorID) {
> +      return (
> +        dom.div({ className: "devtools-sidepanel-no-result" },
> +          getStr("flexbox.noFlexboxeOnThisPage")

s/noFlexboxeOnThisPage/noFlexboxOnThisPage

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 9002349 [details] [diff] [review]
Part 3: Show the list of flex container properites in the Flexbox panel. [1.0]

Review of attachment 9002349 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/components/FlexContainerProperties.js
@@ +39,5 @@
> +    });
> +  }
> +
> +  render() {
> +    const { properties } =  this.props;

Since you added the ability to get computed properties from the server in a previous patch, do you think it makes sense to check that the object here is not empty (the default state from flexbox store) in the situations where the inspected server is older and does not have that implementation yet?

@@ +47,5 @@
> +        {
> +          id: "flex-container-properties",
> +          className: "flexbox-container",
> +        },
> +        dom.div(

Replacing this structure with dom.details() and dom.summary() for the corresponding `<details>` and `<summary>` HTML elements will provide the toggling behaviour for free and it will be keyboard accessible by default.

An example here: https://searchfox.org/mozilla-central/source/devtools/client/inspector/fonts/components/FontEditor.js#110-116

::: devtools/client/inspector/layout/components/ComputedProperty.js
@@ +32,5 @@
>      this.renderReferenceElementPreview = this.renderReferenceElementPreview.bind(this);
> +  }
> +
> +  onFocus() {
> +    this.container.focus();

What's the reason for this focus swap?

Comment 7

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

Comment on attachment 9002313 [details] [diff] [review]
Part 1: Refactor the flex container listing out of the Flexbox Component into FlexContainerList. [1.0]

Review of attachment 9002313 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/components/FlexContainerItem.js
@@ +21,3 @@
>    static get propTypes() {
>      return {
>        flexbox: PropTypes.shape(Types.flexbox).isRequired,

We actually use all of the flexbox properties in here - actorID, highlighted, color and nodeFront.

::: devtools/client/inspector/flexbox/components/Flexbox.js
@@ +17,5 @@
>  
>  class Flexbox extends PureComponent {
>    static get propTypes() {
>      return {
>        flexbox: PropTypes.shape(Types.flexbox).isRequired,

It turns out I forgot to explicitly specify some of the props in LayoutApp when I was adding the Flexbox components.

@@ +34,1 @@
>        getSwatchColorPickerTooltip,

Fixed.

@@ +35,5 @@
>        setSelectedNode,
>        onHideBoxModelHighlighter,
>        onSetFlexboxOverlayColor,
>        onShowBoxModelHighlighterForNode,
>        onToggleFlexboxHighlighter,

Fixed.

@@ +41,5 @@
>  
> +    if (!flexbox.actorID) {
> +      return (
> +        dom.div({ className: "devtools-sidepanel-no-result" },
> +          getStr("flexbox.noFlexboxeOnThisPage")

Ahahah good catch. Unfortunately, that typo already made it into the layout.properties from before. To avoid retranslating that string because of changing the key to that string, I have to skip this fix.

Comment 8

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

Comment on attachment 9002349 [details] [diff] [review]
Part 3: Show the list of flex container properites in the Flexbox panel. [1.0]

Review of attachment 9002349 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/flexbox/components/FlexContainerProperties.js
@@ +39,5 @@
> +    });
> +  }
> +
> +  render() {
> +    const { properties } =  this.props;

I think we will be safe here since the panel isn't enabled.

@@ +47,5 @@
> +        {
> +          id: "flex-container-properties",
> +          className: "flexbox-container",
> +        },
> +        dom.div(

This is really good. I think we might want to change the list-style arrow to match our expander. So, I am gonna move this work to another bug.

::: devtools/client/inspector/layout/components/ComputedProperty.js
@@ +32,5 @@
>      this.renderReferenceElementPreview = this.renderReferenceElementPreview.bind(this);
> +  }
> +
> +  onFocus() {
> +    this.container.focus();

Alphabetically sorting the methods.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2f2f59a9ec
Part 1: Refactor the flex container listing out of the Flexbox Component into FlexContainerList. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3d9ddf714d5
Part 2: Return the computed style properties of the flex container from the FlexboxActor. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d02165bc767
Part 3: Show the list of flex container properites in the Flexbox panel. r=rcaliman

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8b2f2f59a9ec
https://hg.mozilla.org/mozilla-central/rev/b3d9ddf714d5
https://hg.mozilla.org/mozilla-central/rev/5d02165bc767