Closed
Bug 1414362
Opened 7 years ago
Closed 7 years ago
Outline the flex container and items 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: gl)
References
(Blocks 1 open bug)
Details
(Whiteboard: [designer-tools])
Attachments
(1 file)
No description provided.
Whiteboard: [designer-tools]
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
Gabriel, I pulled this from mozreview and also applied the patches from bug 1414275 to test this, but couldn't get it to work. I couldn't see anything in the ruleview for display:flex elements. What is the best way for me to test this locally?
Flags: needinfo?(gl)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.
https://reviewboard.mozilla.org/r/198136/#review203714
`_hasMoved` will probably need to be overridden in this class too, so that we can react to alignment and size changes (currently the highlighter does not).
This can lead to weird cases where the container resizes fine, but not the items. See this for instance: https://www.dropbox.com/s/obciw6vepdmtc9y/inline-flex-wrapping.png?dl=0
::: devtools/server/actors/highlighters/flexbox.js:25
(Diff revision 1)
> + getAdjustedQuads,
> + getDisplayPixelRatio,
> setIgnoreLayoutChanges,
> } = require("devtools/shared/layout/utils");
>
> +const FLEXBOX_DEFAULT_COLOR = "#9400FF";
Let's take the opportunity to move this to a shared place so the grid highlighter can also use it. We try very hard to define colors in css variables for css, so it would be a pity if we started duplicating colors in JS now.
::: devtools/server/actors/highlighters/flexbox.js:202
(Diff revision 1)
> + this.ctx.lineWidth = lineWidth;
> + this.ctx.strokeStyle = FLEXBOX_DEFAULT_COLOR;
> +
> + let { bounds } = this.currentQuads.content[0];
> +
> + drawRect(this.ctx, 0, 0, bounds.right - bounds.left, bounds.bottom - bounds.top,
Can't we just use `bounds.width` and `bounds.height`?
::: devtools/server/actors/highlighters/flexbox.js:229
(Diff revision 1)
> + this.ctx.globalAlpha = FLEXBOX_LINES_PROPERTIES.item.alpha;
> + this.ctx.lineWidth = lineWidth;
> + this.ctx.strokeStyle = FLEXBOX_DEFAULT_COLOR;
> +
> + let { bounds } = this.currentQuads.content[0];
> + let flexItems = this.currentNode.children;
can you add a TODO comment explaining that this will soon come from a platform API instead, because right now this doesn't work in all cases.
::: devtools/server/actors/highlighters/flexbox.js:232
(Diff revision 1)
> +
> + let { bounds } = this.currentQuads.content[0];
> + let flexItems = this.currentNode.children;
> +
> + for (let flexItem of flexItems) {
> + let quads = getAdjustedQuads(this.win, flexItem, "content");
And in fact, the platform API should return the "offset" for each item (much like we do for grid) so we don't have to cacuate geometry again here. Also, some items are not element nodes, so it may not always be possible for us to access the quads.
Attachment #8926886 -
Flags: review?(pbrosset)
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.
https://reviewboard.mozilla.org/r/198136/#review204096
Attachment #8926886 -
Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f2e48e66d
Outline the flex container and items in the flexbox highlighter. r=pbro
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•