1308268 - Add a grid outline in the grid layout panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

Adds a grid outline in the grid layout panel. To keep this simple we should only display the grid outline if the current selected node is a grid container or child of a grid container and not deal with overlaying multiple grids in this outline view.

The grid outline is the small representation of the grid and should reflect grid as it changes with different viewport sizes.

Comment 1

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

Attachment: 1308268.patch

Comment 2

[Patrick Brosset <:pbro>]

I took at look at this WIP patch and I thought to myself, why not use CSS grid to create the outline? After all we're drawing a grid, so why not use an actual CSS grid.
But then I realized all the data we had at our disposal was what getGridFragments returns, so we don't really have a choice but to draw the grid with something else.
However, why not use SVG for this? I think we'll need to have some kind of mouseover event handling at some stage to highlight areas and cells, right? SVG would make that easier than canvas.

Comment 3

[Micah Tigley [:micah]]

Attachment: Bug1308268v1.patch

First iteration of the grid outline

Comment 4

[Micah Tigley [:micah]]

Attachment: Bug1308268v2.patch

Adds a mouseover event for each cell. The event simply highlights the hovered cell.

Comment 5

[Micah Tigley [:micah]]

Attachment: Bug1308268v3.patch

Removes most linter errors. Remove the extra columns and rows being rendered.

Comment 6

[Micah Tigley [:micah]]

Attachment: Bug1308268v4.patch

Squash commits

Comment 7

[Micah Tigley [:micah]]

Attachment: Bug1308268v5.patch

This patch draws a another svg rectangle to act as a border for the inside cells on the grid outline.

Comment 8

[Micah Tigley [:micah]]

Attachment: Bug1308268v6.patch

This patch fixes weird onmouseover event behaviours.

Comment 9

[Micah Tigley [:micah]]

Attachment: Bug1308268v7.patch

v6 patch had the cols and rows reversed when drawing the grid cells. This patch fixes it.

Comment 10

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

Comment on attachment 8834566 [details] [diff] [review]
Bug1308268v7.patch

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

Only added some notes on coding style. The grid outline should not take up any space if no grids are highlighted. 

I am passing this over to pbro to provide some better feedback on the svg rendering. I think we need to fix the overlapping lines (maybe 'stroke-width: 1' will fix this). I am gonna take some more time to play around with this to get it to look better and learn svg as well. In the meantime, I am hoping pbro can provide more guidance.

::: devtools/client/inspector/layout/components/GridOutline.js
@@ +19,5 @@
> +
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  getInitialState() {
> +    return { selectedGrids: [] };

Format this to be:

return {
  selectedGrids: [],
};

@@ +22,5 @@
> +  getInitialState() {
> +    return { selectedGrids: [] };
> +  },
> +
> +  componentWillReceiveProps({grids}) {

Format this to put spaces after '{' and before '}'.

componentWillReceiveProps({ grids }) { .. }

@@ +23,5 @@
> +    return { selectedGrids: [] };
> +  },
> +
> +  componentWillReceiveProps({grids}) {
> +    this.setState({selectedGrids: this.processGridsToDraw(grids)});

Format this to be:
this.setState({
  selectedGrids: this.processGridsToDraw(grids),
});

@@ +26,5 @@
> +  componentWillReceiveProps({grids}) {
> +    this.setState({selectedGrids: this.processGridsToDraw(grids)});
> +  },
> +
> +  processGridsToDraw(grids) {

I would like to reorder these functions inside the GridOutline component, so that it is slightly alphabetically ordered after the typical React class propoerties:

displayName,
propTypes,
mixins,
getInitialState,
componentWillReceiveProps,
draw,
drawGridOutlineBorder,
processGridsToDraw,
renderCell,
renderFragment,
onMouseLeaveCell,
onMouseOverCell,
render

There are a couple of exceptions - render is also the last function and handler functions onX, onY are always grouped up alphabetically before render().

@@ +54,5 @@
> +    const numberOfRowLines = rows.lines.length - 1;
> +
> +    const rectangles = [];
> +
> +    // We will draw a rectangle that acts as a border for the grid outline

s/We will draw/Draw

@@ +69,5 @@
> +
> +    return rectangles;
> +  },
> +
> +  onMouseOverCell({target}) {

Format: { target }

@@ +73,5 @@
> +  onMouseOverCell({target}) {
> +    target.setAttribute("fill", "#DED0FE");
> +  },
> +
> +  onMouseLeaveCell({target}) {

Format: { target }

Comment 11

[Micah Tigley [:micah]]

Attachment: Bug1308268v8.patch

Fix <rect> x coordinate position for each cell. Applied requested changes to code as specified in previous review.

Comment 12

[Micah Tigley [:micah]]

Attachment: Bug 1308268 - Use thinner stroke widths for grid outline.

Review commit: https://reviewboard.mozilla.org/r/111402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111402/

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8835707 [details] [diff] [review]
Bug1308268v8.patch

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

Thanks for the patch.
Looks pretty good.
I have made a few comments in the code below.
And also, found that the outline was sometimes cut off, not displayed in full. This is most probably due to a problem with the viewBox and preserveAspectRatio attributes.

::: devtools/client/inspector/layout/components/GridOutline.js
@@ +37,5 @@
> +    return dom.g(
> +      {
> +        id: "grid-cell-group",
> +      },
> +      fragmentsToRender

Instead of doing a for loop before the return statement, could you do something like this:

return dom.g({ id: "grid-cell-group" },
  gridFragments.map(gridFragment => this.renderFragment(gridFragment))
);

Also, please use classes instead of ids. We might want to display more than 1 outline in the future.

@@ +49,5 @@
> +        x: 1,
> +        y: 1,
> +        width: numberOfCols * 10,
> +        height: numberOfRows * 10,
> +        stroke: "#9370DB",

Please don't add colors here. You can define them in CSS instead. And even use theme css variables if possible.
If we don't have variables for this, then you could create new ones. It makes it easier to maintain, and customize with other themes.

@@ +55,5 @@
> +      }
> +    );
> +  },
> +
> +  processGridsToDraw(grids) {

Please add some jsdoc comment for this method to make it clear what it's responsibility is.

@@ +62,5 @@
> +    for (let grid of grids) {
> +      if (!grid.highlighted) {
> +        continue;
> +      }
> +      highlightedGrids.push(grid.gridFragments[0]);

Are we choosing to draw only the first fragment on purpose here? I assume that it's the case, and we will handle fragmented grids in a later bug. This is fine, but please do add a // TODO comment here telling people that we chose to do this at first but should really iterate over all fragments in the future.

@@ +69,5 @@
> +  },
> +
> +  renderCell(fragment, columnNumber, rowNumber) {
> +    const cellId =
> +      `grid-column:${columnNumber}-row:${rowNumber}-fragment:${fragment.number}`;

What is the purpose of this complicated ID? I haven't seen it used anywhere else in the code, and it's not for CSS styling.
I suggest removing it.

@@ +79,5 @@
> +        width: 10,
> +        height: 10,
> +        onMouseOver: this.onMouseOverCell,
> +        onMouseOut: this.onMouseLeaveCell,
> +        stroke: "#9370DB",

Same comment as before: please move this to the CSS file instead, by using a class.

@@ +80,5 @@
> +        height: 10,
> +        onMouseOver: this.onMouseOverCell,
> +        onMouseOut: this.onMouseLeaveCell,
> +        stroke: "#9370DB",
> +        fill: "none",

This can also be added to the CSS file instead.

@@ +81,5 @@
> +        onMouseOver: this.onMouseOverCell,
> +        onMouseOut: this.onMouseLeaveCell,
> +        stroke: "#9370DB",
> +        fill: "none",
> +        pointerEvents: "all",

This can also be added to the CSS file instead.

@@ +82,5 @@
> +        onMouseOut: this.onMouseLeaveCell,
> +        stroke: "#9370DB",
> +        fill: "none",
> +        pointerEvents: "all",
> +        strokeDasharray: [2, 2],

This can also be added to the CSS file instead.

@@ +123,5 @@
> +    target.setAttribute("fill", "none");
> +  },
> +
> +  onMouseOverCell({ target }) {
> +    target.setAttribute("fill", "#DED0FE");

It doesn't look like you need the mouseleave/mouseover event handlers here. All you're doing is changing the background color, and you can do that using the :hover pseudo-class in CSS instead.

@@ +132,5 @@
> +      {
> +        id: "grid-outline",
> +        width: this.state.selectedGrids.length ? 100 : 0,
> +        height: this.state.selectedGrids.length ? 100 : 0,
> +        viewBox: "0 0 60 55",

60 and 55 are "magic numbers". Why did we choose them? What's their importance?
Please move them as const at the top of the file, with a self-explanatory name and some comment explaining why it's important that they are 60 and 55.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8835823 [details]
Bug 1308268 - Use thinner stroke widths for grid outline.

https://reviewboard.mozilla.org/r/111402/#review113300

::: devtools/client/inspector/layout/components/GridOutline.js:53
(Diff revision 1)
>          id: "grid-outline-border",
>          x: 1,
>          y: 1,
>          width: numberOfCols * 10,
>          height: numberOfRows * 10,
> +        strokeWidth: 0.75,

Please move this to the css file as well. And I think you should fold the 2 patches. This is a very small change that should go in the previous patch.

Comment 15

[Micah Tigley [:micah]]

Attachment: Bug1308268.patch

Comment 16

[Micah Tigley [:micah]]

Attachment: Bug1308268.patch

Comment 17

[Micah Tigley [:micah]]

Attachment: Bug1308268.patch

Comment 18

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

Comment on attachment 8839063 [details] [diff] [review]
Bug1308268.patch

Needs to rebased.

Comment 19

[Micah Tigley [:micah]]

Attachment: Bug 1308268 - Add a Grid Outline to layout panel.

Review commit: https://reviewboard.mozilla.org/r/114818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114818/

Comment 20

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

Comment on attachment 8840316 [details]
Bug 1308268 - Add a Grid Outline to layout panel.

https://reviewboard.mozilla.org/r/114818/#review116544

::: devtools/client/inspector/layout/components/Grid.js:52
(Diff revision 1)
>            id: "layout-grid-container",
>          },
> +        GridOutline({
> +          grids,
> +        }),
>          GridList({

We are missing the div.grid-content here.

::: devtools/client/inspector/layout/components/GridOutline.js:14
(Diff revision 1)
> +
> +const Types = require("../types");
> +
> +// Move SVG grid to the right 100 units, so that it is not flushed against the edge of
> +// layout border
> +const TranslateX = -100;

We want to use all capitalizations for constants.

s/TranslateX/TRANSLATE_X

::: devtools/client/inspector/layout/components/GridOutline.js:15
(Diff revision 1)
> +const Types = require("../types");
> +
> +// Move SVG grid to the right 100 units, so that it is not flushed against the edge of
> +// layout border
> +const TranslateX = -100;
> +const TranslateY = 0;

TRANSLATE_Y

::: devtools/client/inspector/layout/components/GridOutline.js:17
(Diff revision 1)
> +// Move SVG grid to the right 100 units, so that it is not flushed against the edge of
> +// layout border
> +const TranslateX = -100;
> +const TranslateY = 0;
> +
> +const ViewportWidth = 450;

VIEWPORT_WIDTH

::: devtools/client/inspector/layout/components/GridOutline.js:18
(Diff revision 1)
> +// layout border
> +const TranslateX = -100;
> +const TranslateY = 0;
> +
> +const ViewportWidth = 450;
> +const ViewportHeight = 100;

VIEWPORT_HEIGHT

Move this above VIEWPORT_WIDTH to keep it alphabetically ordered.

::: devtools/client/inspector/layout/components/GridOutline.js:37
(Diff revision 1)
> +      selectedGrids: [],
> +    };
> +  },
> +
> +  componentWillReceiveProps({ grids }) {
> +    this.setState({selectedGrids: this.processGridsToDraw(grids)});

Add spaces between opening { and closing }

::: devtools/client/inspector/layout/components/GridOutline.js:40
(Diff revision 1)
> +
> +  componentWillReceiveProps({ grids }) {
> +    this.setState({selectedGrids: this.processGridsToDraw(grids)});
> +  },
> +
> +  draw(gridFragments) {

We should be consistent with using the word "draw" or "render". Since we already use render in a bunch of places, let's just replace draw with render.

s/draw/renderGridOutline.

::: devtools/client/inspector/layout/components/GridOutline.js:49
(Diff revision 1)
> +      },
> +      gridFragments.map(gridFragment => this.renderFragment(gridFragment))
> +    );
> +  },
> +
> +  drawGridOutlineBorder(numberOfRows, numberOfCols) {

s/drawGridOutlineBorder/renderGridOutlineBorder

::: devtools/client/inspector/layout/components/GridOutline.js:56
(Diff revision 1)
> +      {
> +        className: "grid-outline-border",
> +        x: 1,
> +        y: 1,
> +        width: numberOfCols * 10,
> +        height: numberOfRows * 10,

We should pull the grid color and add the stroke color here. We recently added a color picker for each individual grid. So, we should be rendering the outline using the color that is selected for that grid item.

::: devtools/client/inspector/layout/components/GridOutline.js:62
(Diff revision 1)
> +      }
> +    );
> +  },
> +
> +  /**
> +  * processGridsToDraw - Determines what grid fragments to draw in the grid outline

Remove "processGridsToDraw - " and maybe phrase the comment to be:
Return the grid fragments of the highlighted grid containers.

::: devtools/client/inspector/layout/components/GridOutline.js:64
(Diff revision 1)
> +  },
> +
> +  /**
> +  * processGridsToDraw - Determines what grid fragments to draw in the grid outline
> +  * by storing the fragment data for every highlighted grid
> +  * @param  {array} grids - List of grid objects returned from the redux store

Add a new empty line to seperate the comment and @param.

::: devtools/client/inspector/layout/components/GridOutline.js:64
(Diff revision 1)
> +  },
> +
> +  /**
> +  * processGridsToDraw - Determines what grid fragments to draw in the grid outline
> +  * by storing the fragment data for every highlighted grid
> +  * @param  {array} grids - List of grid objects returned from the redux store

s/array/Array. See inspector/client/layout/layout.js on how we format JSDocs. 

The @param description should be formatted on the next line.

The grids description should be:
Array of grid container objects on the page.

::: devtools/client/inspector/layout/components/GridOutline.js:65
(Diff revision 1)
> +
> +  /**
> +  * processGridsToDraw - Determines what grid fragments to draw in the grid outline
> +  * by storing the fragment data for every highlighted grid
> +  * @param  {array} grids - List of grid objects returned from the redux store
> +  * @return {array} highlightedGrids -

s/array/Array.
Remove "highlightedGrids -"

::: devtools/client/inspector/layout/components/GridOutline.js:66
(Diff revision 1)
> +  /**
> +  * processGridsToDraw - Determines what grid fragments to draw in the grid outline
> +  * by storing the fragment data for every highlighted grid
> +  * @param  {array} grids - List of grid objects returned from the redux store
> +  * @return {array} highlightedGrids -
> +  * List of fragment data objects for each highlighted grid

Move this to the previous line as:
@return {Array} array of grid fragment data objects for each highlighted grid.

::: devtools/client/inspector/layout/components/GridOutline.js:68
(Diff revision 1)
> +  * by storing the fragment data for every highlighted grid
> +  * @param  {array} grids - List of grid objects returned from the redux store
> +  * @return {array} highlightedGrids -
> +  * List of fragment data objects for each highlighted grid
> +  */
> +

Remove this new line that seperates the JSDoc and function.

::: devtools/client/inspector/layout/components/GridOutline.js:69
(Diff revision 1)
> +  * @param  {array} grids - List of grid objects returned from the redux store
> +  * @return {array} highlightedGrids -
> +  * List of fragment data objects for each highlighted grid
> +  */
> +
> +  processGridsToDraw(grids) {

I tend to avoid functions that are called processX. Rather we should simply state what it returns as the function name. 

s/processGridsToDraw/getHighlightedGridFragments

::: devtools/client/inspector/layout/components/GridOutline.js:79
(Diff revision 1)
> +        continue;
> +      }
> +      // TODO: We are drawing the first fragment since only one is currently being stored.
> +      // In future we will need to iterate over all fragments of a grid.
> +      highlightedGrids.push(grid.gridFragments[0]);
> +    }

Add a new line after the for loop to seperate it

::: devtools/client/inspector/layout/components/GridOutline.js:83
(Diff revision 1)
> +      highlightedGrids.push(grid.gridFragments[0]);
> +    }
> +    return highlightedGrids;
> +  },
> +
> +  renderCell(fragment, columnNumber, rowNumber) {

Remove fragment argument since it is unused.

::: devtools/client/inspector/layout/components/GridOutline.js:83
(Diff revision 1)
> +      highlightedGrids.push(grid.gridFragments[0]);
> +    }
> +    return highlightedGrids;
> +  },
> +
> +  renderCell(fragment, columnNumber, rowNumber) {

s/renderCell/renderGridCell

::: devtools/client/inspector/layout/components/GridOutline.js:84
(Diff revision 1)
> +    }
> +    return highlightedGrids;
> +  },
> +
> +  renderCell(fragment, columnNumber, rowNumber) {
> +    return dom.rect(

Need to add stroke color with the grid item color.

::: devtools/client/inspector/layout/components/GridOutline.js:91
(Diff revision 1)
> +        className: "grid-outline-cell",
> +        x: columnNumber,
> +        y: rowNumber,
> +        width: 10,
> +        height: 10,
> +        onMouseOver: this.onMouseOverCell,

Remove onMouseOver for now and move this over to the mouseover patch.

::: devtools/client/inspector/layout/components/GridOutline.js:96
(Diff revision 1)
> +        onMouseOver: this.onMouseOverCell,
> +      }
> +    );
> +  },
> +
> +  renderFragment(gridFragment) {

s/renderFragment/renderGridFragment.

::: devtools/client/inspector/layout/components/GridOutline.js:109
(Diff revision 1)
> +    // Draw a rectangle that acts as a border for the grid outline
> +    const border = this.drawGridOutlineBorder(numberOfRowLines, numberOfColLines);
> +
> +    rectangles.push(border);
> +
> +    let xpos = 1;

s/xpos/x

::: devtools/client/inspector/layout/components/GridOutline.js:110
(Diff revision 1)
> +    const border = this.drawGridOutlineBorder(numberOfRowLines, numberOfColLines);
> +
> +    rectangles.push(border);
> +
> +    let xpos = 1;
> +    let ypos = 1;

s/ypos/y

::: devtools/client/inspector/layout/components/GridOutline.js:117
(Diff revision 1)
> +    const height = 10;
> +
> +    // Draw the cells within the grid outline border
> +    for (let row = 0; row < numberOfRowLines; row++) {
> +      for (let col = 0; col < numberOfColLines; col++) {
> +        rectangles.push(this.renderCell(cols.lines[col], xpos, ypos));

Can remove cols.lines[col] since this fragment isn't used in renderCell.

::: devtools/client/inspector/layout/components/GridOutline.js:119
(Diff revision 1)
> +    // Draw the cells within the grid outline border
> +    for (let row = 0; row < numberOfRowLines; row++) {
> +      for (let col = 0; col < numberOfColLines; col++) {
> +        rectangles.push(this.renderCell(cols.lines[col], xpos, ypos));
> +        xpos += width;
> +      }

Add a new line after the for loop block

::: devtools/client/inspector/layout/components/GridOutline.js:127
(Diff revision 1)
> +    }
> +
> +    return rectangles;
> +  },
> +
> +  onMouseOverCell({ target }) {

Remove onMouseOverCell.

We want to keep a seperation of concerns between each bug. Since this bug is limited to simply adding the outline, we should not being adding any mouseover handler in this patch and move this over to the mouseover bug.

::: devtools/client/inspector/layout/components/GridOutline.js:133
(Diff revision 1)
> +    // TODO: We will want onMouseOver interactions with the grid cells, which is addressed
> +    // in Bug 1337235
> +  },
> +
> +  render() {
> +    return dom.svg(

We can do:

return this.state.selectedGrids.length ?
  dom.svg(
    {
      ...
    }
  )
  :
  null;

instead.

::: devtools/client/inspector/layout/components/GridOutline.js:136
(Diff revision 1)
> +
> +  render() {
> +    return dom.svg(
> +      {
> +        className: "grid-outline",
> +        width: this.state.selectedGrids.length ? "100%" : 0,

We can remove this check and simply say width: 100% with the above change.

::: devtools/client/inspector/layout/components/GridOutline.js:137
(Diff revision 1)
> +  render() {
> +    return dom.svg(
> +      {
> +        className: "grid-outline",
> +        width: this.state.selectedGrids.length ? "100%" : 0,
> +        height: this.state.selectedGrids.length ? 100 : 0,

height: 100,

::: devtools/client/themes/layout.css:65
(Diff revision 1)
> +#grid-outline {
> +  margin: 5px auto;
> +}
> +
> +.grid-outline-border {
> +  stroke: #9370DB;

Remove stroke.

::: devtools/client/themes/layout.css:73
(Diff revision 1)
> +}
> +
> +.grid-outline-cell {
> +  fill: none;
> +  pointer-events: all;
> +  stroke: #9370DB;

Remove stroke.

::: devtools/client/themes/layout.css:77
(Diff revision 1)
> +  pointer-events: all;
> +  stroke: #9370DB;
> +  stroke-dasharray: 0.5, 2;
> +}
> +
> +.grid-outline-cell:hover {

Move this over to the mouseover bug. Remove from this one.

Comment 21

[Micah Tigley [:micah]]

Comment on attachment 8840316 [details]
Bug 1308268 - Add a Grid Outline to layout panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114818/diff/1-2/

Comment 22

[Micah Tigley [:micah]]

Comment on attachment 8840316 [details]
Bug 1308268 - Add a Grid Outline to layout panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114818/diff/2-3/

Comment 23

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

Comment on attachment 8840316 [details]
Bug 1308268 - Add a Grid Outline to layout panel.

https://reviewboard.mozilla.org/r/114818/#review116642

::: devtools/client/inspector/layout/components/Grid.js:52
(Diff revision 3)
>        dom.div(
>          {
>            id: "layout-grid-container",
>          },
> +        showGridOutline ?
> +         GridOutline({

It looks like there is only an identation of 1 space, but it needs 2 spaces

::: devtools/client/inspector/layout/components/Grid.js:53
(Diff revision 3)
>          {
>            id: "layout-grid-container",
>          },
> +        showGridOutline ?
> +         GridOutline({
> +          grids,

Same as above

::: devtools/client/inspector/layout/components/Grid.js:55
(Diff revision 3)
>          },
> +        showGridOutline ?
> +         GridOutline({
> +          grids,
> +         })
> +        :

This should be indented

::: devtools/client/inspector/layout/components/Grid.js:56
(Diff revision 3)
> +        showGridOutline ?
> +         GridOutline({
> +          grids,
> +         })
> +        :
> +        null,

Same as above.

::: devtools/client/inspector/layout/components/Grid.js:59
(Diff revision 3)
> +         })
> +        :
> +        null,
> +        dom.div(
> +          {
> +          className: "grid-content",

This needs to be indented

::: devtools/client/inspector/layout/components/GridOutline.js:37
(Diff revision 3)
> +      selectedGrids: [],
> +    };
> +  },
> +
> +  componentWillReceiveProps({ grids }) {
> +    this.setState({ selectedGrids: this.getHighlightedGridFragments(grids) });

Let's format similar to getInitialState():
this.setState({
  selectedGrids: this.getHighlightedGridFragments(grids),
});

::: devtools/client/inspector/layout/components/GridOutline.js:43
(Diff revision 3)
> +  },
> +
> +  /**
> +  * Return the grid fragments of the highlighted grid containers.
> +  *
> +  * @param    {Array} grids

Should be 2 spaces after @param. You will see {Array} lines up with the return {Array}

::: devtools/client/inspector/layout/components/GridOutline.js:44
(Diff revision 3)
> +
> +  /**
> +  * Return the grid fragments of the highlighted grid containers.
> +  *
> +  * @param    {Array} grids
> +  *           Array of grid container objects on the page

Indentation here will need to be realigned when you fix the above.

::: devtools/client/inspector/layout/components/GridOutline.js:45
(Diff revision 3)
> +  /**
> +  * Return the grid fragments of the highlighted grid containers.
> +  *
> +  * @param    {Array} grids
> +  *           Array of grid container objects on the page
> +  * @return   {Array}

1 space after @return

::: devtools/client/inspector/layout/components/GridOutline.js:49
(Diff revision 3)
> +  *           Array of grid container objects on the page
> +  * @return   {Array}
> +  *           Array of grid fragment data objects for each highlighted grid.
> +  */
> +  getHighlightedGridFragments(grids) {
> +    const highlightedGrids = [];

We can reduce this function to Array.filter():

return grids.filter(grid => grid.highlighted);

We can probably remove this function and just call this in componentWillReceiveProps

::: devtools/client/inspector/layout/components/GridOutline.js:79
(Diff revision 3)
> +
> +  renderGridFragment({ color, gridFragments }) {
> +    // TODO: We are drawing the first fragment since only one is currently being stored.
> +    // In future we will need to iterate over all fragments of a grid.
> +    const { rows, cols } = gridFragments[0];
> +

Remove this new line.

::: devtools/client/inspector/layout/components/GridOutline.js:82
(Diff revision 3)
> +    // In future we will need to iterate over all fragments of a grid.
> +    const { rows, cols } = gridFragments[0];
> +
> +    const numOfColLines = cols.lines.length - 1;
> +    const numOfRowLines = rows.lines.length - 1;
> +

Remove this new line

::: devtools/client/inspector/layout/components/GridOutline.js:87
(Diff revision 3)
> +
> +    const rectangles = [];
> +
> +    // Draw a rectangle that acts as a border for the grid outline
> +    const border = this.renderGridOutlineBorder(numOfRowLines, numOfColLines, color);
> +

Remove this new line

Comment 24

[Micah Tigley [:micah]]

Comment on attachment 8840316 [details]
Bug 1308268 - Add a Grid Outline to layout panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114818/diff/3-4/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aededf5a3d38
Add a Grid Outline to layout panel. r=gl

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aededf5a3d38

Comment 27

[Tanvir Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-10-06) on Windows 10, 64 bit!

The bug's fix is now verified on Latest Beta 54.0b3

Build ID 	20170427091925
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170426]