1431841 - Show the flex lines in the flexbox highlighter

Status: RESOLVED FIXED

Description

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

Comment 1

[Shuoyi Ma]

Created attachment 8944076 [details] [diff] [review]
1431841.patch

Comment 2

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

Comment on attachment 8944076 [details] [diff] [review]
1431841.patch

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

::: devtools/server/actors/highlighters/flexbox.js
@@ +31,1 @@
>      alpha: 1,

Global alpha actually defaults to 1. So, I think we can actually remove all the alpha and all the globalAlpha that we set.

@@ +31,3 @@
>      alpha: 1,
>    },
> +  "line": {

This won't be needed because of the above.

@@ +288,5 @@
>      this.ctx.stroke();
>      this.ctx.restore();
>    }
>  
> +  renderFlexContainerBorder() {

Move this function above renderFlexContainerFill. We actually order these functions alphabetically.

@@ +317,5 @@
> +
> +  renderFlexLines() {
> +    if (!this.currentQuads.content || !this.currentQuads.content[0]) {
> +      return;
> +    }

Add a new line after if statement blocks

@@ +319,5 @@
> +    if (!this.currentQuads.content || !this.currentQuads.content[0]) {
> +      return;
> +    }
> +    let container = this.currentNode;
> +    let containerStyle = getComputedStyle(container);

s/containerStyle/computedStyle

Just use this.currentNode instead of assigning it to "container.

@@ +321,5 @@
> +    }
> +    let container = this.currentNode;
> +    let containerStyle = getComputedStyle(container);
> +    let flexLines = container.getAsFlexContainer().getLines();
> +    if(!flexLines) {

Add a space after "if"

@@ +339,5 @@
> +    this.ctx.lineWidth = lineWidth;
> +    this.ctx.strokeStyle = DEFAULT_COLOR;
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;

This is never used.

@@ +341,5 @@
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {

This is eslint error since it runs over 90 characters in a line.

@@ +342,5 @@
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {
> +      for (let flexLineItem of flexLines) {

s/flexLineItem/flexLine

@@ +343,5 @@
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);

We can destructure flexLine and get the crossStart and crossSize.

const { crossStart, crossSize } = flexLine;

@@ +346,5 @@
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);
> +
> +        drawRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart, bounds.height, this.currentMatrix);
> +        this.ctx.stroke();

I am wondering if we can just call stroke once. Double check for me.

@@ +351,5 @@
> +        drawRect(this.ctx, flexLineItem.crossStart +  flexLineItem.crossSize, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);
> +        this.ctx.stroke();
> +       }
> +    } else {
> +      for (let flexLineItem of flexLines) {

We can probably just check the computedStyle.getPropertyValue("flex-direction") inside the for loop so we can just have one for loop

for (let flexLine of flexLines) {
 if (column) {
  ....
 } else {
  ....
 }
}

@@ +355,5 @@
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, 0,flexLineItem.crossStart, bounds.width, flexLineItem.crossStart +  flexLineItem.crossSize, this.currentMatrix);
> +
> +        drawRect(this.ctx, 0, flexLineItem.crossStart, bounds.width, flexLineItem.crossStart, this.currentMatrix);
> +        this.ctx.stroke();

Probably can just call stroke once() as above.

@@ +363,5 @@
> +    }
> +
> +    this.ctx.restore();
> +
> +    console.log(flexLines)

Remove this console.log

Comment 3

[Shuoyi Ma]

Created attachment 8944184 [details] [diff] [review]
1431841.patch

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82427fc508cd
Show the flex lines in the flexbox highlighter. r=gl

Comment 5

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

Note to sheriff, I think the eslint errors in Bug 1431941.

Comment 6

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/mozilla-central/rev/82427fc508cd