Closed
Bug 1431841
Opened 5 years ago
Closed 5 years ago
Show the flex lines in the flexbox highlighter
Categories
(DevTools :: Inspector, enhancement, P3)
DevTools
Inspector
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: gl, Assigned: mashuoyi111)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
6.77 KB,
patch
|
gl
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8944076 -
Flags: review?(gl)
Reporter | ||
Comment 2•5 years ago
|
||
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
Attachment #8944076 -
Flags: review?(gl)
Attachment #8944184 -
Flags: review?(gl)
Attachment #8944076 -
Attachment is obsolete: true
Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/82427fc508cd Show the flex lines in the flexbox highlighter. r=gl
Reporter | ||
Comment 5•5 years ago
|
||
Note to sheriff, I think the eslint errors in Bug 1431941.
Reporter | ||
Updated•5 years ago
|
Attachment #8944184 -
Flags: review?(gl) → review+
Comment 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82427fc508cd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•