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.

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.

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.

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.

(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 on attachment 8926870 [details] Bug 1414275 - Part 1: Convert HighlightersOverlay to use es6 classes. https://reviewboard.mozilla.org/r/198120/#review204078

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

https://hg.mozilla.org/mozilla-central/rev/e5f002dcdb23 https://hg.mozilla.org/mozilla-central/rev/2fa72fbd3014 https://hg.mozilla.org/mozilla-central/rev/5125625bd2c3