Closed
Bug 1414275
Opened 7 years ago
Closed 7 years ago
Toggle flexbox layout highlighter from the rule view next to 'display: flex|inline-flex' declarations
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
(3 files)
No description provided.
Whiteboard: [designer-tools]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8926870 [details]
Bug 1414275 - Part 1: Convert HighlightersOverlay to use es6 classes.
https://reviewboard.mozilla.org/r/198120/#review203704
Looks great except for the for loop change in destroy.
::: devtools/client/inspector/shared/highlighters-overlay.js:766
(Diff revision 1)
> /**
> * Destroy this overlay instance, removing it from the view and destroying
> * all initialized highlighters.
> */
> - destroy: function () {
> - for (let type in this.highlighters) {
> + destroy() {
> + for (let type of this.highlighters) {
`this.highlighters` is an object, so not iterable. This should remain a `for..in` loop.
Attachment #8926870 -
Flags: review?(pbrosset)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8926871 [details]
Bug 1414275 - Part 2: Toggle flexbox layout highlighter from the rule view next to 'display: flex|inline-flex' declarations.
https://reviewboard.mozilla.org/r/198122/#review203708
::: devtools/client/inspector/shared/highlighters-overlay.js:859
(Diff revision 1)
> // displayed.
> return;
> }
>
> - if (this.gridHighlighterShown) {
> - let nodeFront = this.gridHighlighterShown;
> + this._hideHighlighterIfDeadNode(this.flexboxHighlighterShown,
> + this.hideFlexboxHighlighter);
I think you need to bind the function to `this` here (same for the other 2 highlighters below) before passing it as a reference. Unless ES6 classes don't require this anymore.
::: devtools/client/themes/rules.css:474
(Diff revision 1)
> display: inline-block;
> position: relative;
> }
>
> +.ruleview-flex {
> + background: url("chrome://devtools/skin/images/command-frames.svg");
We will need a new icon specificially for flexbox. Have you looked into this already?
No need to do this in this bug, but we'll need it before shipping the feature.
Attachment #8926871 -
Flags: review?(pbrosset) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8926872 [details]
Bug 1414275 - Part 3: Add unit test for the flexbox highlighter toggle in the rule view.
https://reviewboard.mozilla.org/r/198124/#review203710
Are these a copy/paste of the grid tests? If so, ok. If there are important differences, please let me know so I can take a closer look.
Attachment #8926872 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926871 [details]
Bug 1414275 - Part 2: Toggle flexbox layout highlighter from the rule view next to 'display: flex|inline-flex' declarations.
https://reviewboard.mozilla.org/r/198122/#review203708
> I think you need to bind the function to `this` here (same for the other 2 highlighters below) before passing it as a reference. Unless ES6 classes don't require this anymore.
The functions are binded in the constructor.
> We will need a new icon specificially for flexbox. Have you looked into this already?
> No need to do this in this bug, but we'll need it before shipping the feature.
Not yet. It's hard to think about what a flexbox icon actually looks like so I picked an icon that had boxes.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
(In reply to Gabriel [:gl] (ΦωΦ) from comment #7)
> > We will need a new icon specificially for flexbox. Have you looked into this already?
> > No need to do this in this bug, but we'll need it before shipping the feature.
>
> Not yet. It's hard to think about what a flexbox icon actually looks like so
> I picked an icon that had boxes.
Here are 2 ideas: https://codepen.io/captainbrosset/pen/pdPmWL?editors=1100
The first one is what Webflow uses.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8926870 [details]
Bug 1414275 - Part 1: Convert HighlightersOverlay to use es6 classes.
https://reviewboard.mozilla.org/r/198120/#review204078
Attachment #8926870 -
Flags: review?(pbrosset) → review+
Comment 13•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f002dcdb23
Part 1: Convert HighlightersOverlay to use es6 classes. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa72fbd3014
Part 2: Toggle flexbox layout highlighter from the rule view next to 'display: flex|inline-flex' declarations. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/5125625bd2c3
Part 3: Add unit test for the flexbox highlighter toggle in the rule view. r=pbro
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e5f002dcdb23
https://hg.mozilla.org/mozilla-central/rev/2fa72fbd3014
https://hg.mozilla.org/mozilla-central/rev/5125625bd2c3
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
•