Add a grid outline in the grid layout panel

RESOLVED FIXED in Firefox 54

Status

P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: gl, Assigned: micah)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 54

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 13 obsolete attachments)

59 bytes, text/x-review-board-request
Details
(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1181227
(Reporter)

Updated

2 years ago
Summary: Adds a grid outline in the grid layout panel → Add a grid outline in the grid layout panel
(Reporter)

Updated

2 years ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Assignee: gl → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

2 years ago
Assignee: nobody → tigleym
Status: NEW → ASSIGNED
(Reporter)

Comment 1

2 years ago
Created attachment 8831029 [details] [diff] [review]
1308268.patch
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8831452 [details] [diff] [review]
Bug1308268v1.patch

First iteration of the grid outline
(Assignee)

Comment 4

2 years ago
Created attachment 8831455 [details] [diff] [review]
Bug1308268v2.patch

Adds a mouseover event for each cell. The event simply highlights the hovered cell.
Attachment #8831452 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8831452 - Attachment is obsolete: false
(Reporter)

Updated

2 years ago
Attachment #8831029 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Created attachment 8831494 [details] [diff] [review]
Bug1308268v3.patch

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

Comment 6

2 years ago
Created attachment 8831587 [details] [diff] [review]
Bug1308268v4.patch

Squash commits
Attachment #8831494 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Created attachment 8834218 [details] [diff] [review]
Bug1308268v5.patch

This patch draws a another svg rectangle to act as a border for the inside cells on the grid outline.
Attachment #8831452 - Attachment is obsolete: true
Attachment #8831455 - Attachment is obsolete: true
Attachment #8831587 - Attachment is obsolete: true
Attachment #8834218 - Flags: feedback?(gl)
(Assignee)

Comment 8

2 years ago
Created attachment 8834224 [details] [diff] [review]
Bug1308268v6.patch

This patch fixes weird onmouseover event behaviours.
Attachment #8834218 - Attachment is obsolete: true
Attachment #8834218 - Flags: feedback?(gl)
Attachment #8834224 - Flags: review?(gl)
(Assignee)

Comment 9

2 years ago
Created attachment 8834566 [details] [diff] [review]
Bug1308268v7.patch

v6 patch had the cols and rows reversed when drawing the grid cells. This patch fixes it.
Attachment #8834224 - Attachment is obsolete: true
Attachment #8834224 - Flags: review?(gl)
Attachment #8834566 - Flags: review?(gl)
(Reporter)

Comment 10

2 years ago
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 }
Attachment #8834566 - Flags: review?(gl) → feedback?(pbrosset)
(Assignee)

Comment 11

2 years ago
Created attachment 8835707 [details] [diff] [review]
Bug1308268v8.patch

Fix <rect> x coordinate position for each cell. Applied requested changes to code as specified in previous review.
Attachment #8834566 - Attachment is obsolete: true
Attachment #8834566 - Flags: feedback?(pbrosset)
Attachment #8835707 - Flags: review?(gl)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8835707 - Flags: review?(gl) → review?(pbrosset)
(Reporter)

Updated

2 years ago
Attachment #8835823 - Flags: review?(gl) → review?(pbrosset)
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.
Attachment #8835707 - Flags: review?(pbrosset)

Comment 14

2 years ago
mozreview-review
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.
Attachment #8835823 - Flags: review?(pbrosset)
(Assignee)

Comment 15

2 years ago
Created attachment 8838985 [details] [diff] [review]
Bug1308268.patch
Attachment #8835707 - Attachment is obsolete: true
Attachment #8835823 - Attachment is obsolete: true
Attachment #8838985 - Flags: review?(gl)
(Assignee)

Comment 16

2 years ago
Created attachment 8839053 [details] [diff] [review]
Bug1308268.patch
Attachment #8838985 - Attachment is obsolete: true
Attachment #8838985 - Flags: review?(gl)
Attachment #8839053 - Flags: review?(gl)
(Assignee)

Comment 17

2 years ago
Created attachment 8839063 [details] [diff] [review]
Bug1308268.patch
Attachment #8839053 - Attachment is obsolete: true
Attachment #8839053 - Flags: review?(gl)
Attachment #8839063 - Flags: review?(gl)
(Reporter)

Comment 18

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

Needs to rebased.
Attachment #8839063 - Flags: review?(gl)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8839063 - Attachment is obsolete: true
(Reporter)

Comment 20

2 years ago
mozreview-review
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.
Attachment #8840316 - Flags: review?(gl)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 23

2 years ago
mozreview-review
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
Attachment #8840316 - Flags: review?(gl) → review+
Comment hidden (mozreview-request)

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aededf5a3d38
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 27

2 years ago
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]

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.