1337235 - Add mouseover interaction for the grid outline to display grid area highlights

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Micah Tigley [:micah]]

We need to implement mouseover interactions for the grid outline. This bug should:

1. Figure out the row/col of the mouseover using the grid fragment data.
2. Figure out if there is a grid area, if yes then show the grid highlight.

Comment 1

[Micah Tigley [:micah]]

Attachment: Bug 1337235 - Mouseover interaction for grid outline.

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

Comment 2

[Micah Tigley [:micah]]

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

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

Comment 3

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

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review116294

Comment 4

[Micah Tigley [:micah]]

Attachment: Bug1337235 - Mouseover interactions for grid outline.

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

Comment 5

[Micah Tigley [:micah]]

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

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

Comment 6

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

Comment on attachment 8840322 [details]
Bug1337235 - Mouseover interactions for grid outline.

https://reviewboard.mozilla.org/r/114828/#review116972

::: devtools/client/inspector/layout/components/GridOutline.js:63
(Diff revision 1)
>        }
>      );
>    },
>  
>    /**
> +  * getGridAreaName - Determines if grid cell is part of a grid area and

Apply the JSDoc formatting that you should be used to from the grid outline patch.

::: devtools/client/inspector/layout/components/GridOutline.js:63
(Diff revision 1)
>        }
>      );
>    },
>  
>    /**
> +  * getGridAreaName - Determines if grid cell is part of a grid area and

Comment should be:
Returns the grid area name if the given grid cell is part of a grid area.

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

This is mostly a rebasing thing since it differs from the latest grid outline patch, but I don't think we need the fragment here.

::: devtools/client/inspector/layout/components/GridOutline.js:102
(Diff revision 1)
>      }
>      return highlightedGrids;
>    },
>  
> -  renderCell(fragment, columnNumber, rowNumber) {
> +  renderCell(fragment, columnNumber, rowNumber, areas, nodeId) {
> +    const name = this.getGridAreaName(fragment, areas);

The grid fragment object is different from {row, col} as an object. I would just pass in the individual row and col to not confuse things.

::: devtools/client/inspector/layout/components/GridOutline.js:107
(Diff revision 1)
> +    const name = this.getGridAreaName(fragment, areas);
>      return dom.rect(
>        {
>          className: "grid-outline-cell",
> +        "data-nodeId": nodeId,
> +        "data-name": name,

Put data-name before data-nodeId

::: devtools/client/inspector/layout/components/GridOutline.js:119
(Diff revision 1)
>      );
>    },
>  
> -  renderFragment(gridFragment) {
> -    const { rows, cols } = gridFragment;
> +  renderFragment({gridFragments, id}) {
> +    // 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.

In the future,*

::: devtools/client/inspector/layout/components/GridOutline.js:151
(Diff revision 1)
>  
>      return rectangles;
>    },
>  
>    onMouseOverCell({ target }) {
> -    // TODO: We will want onMouseOver interactions with the grid cells, which is addressed
> +    const { grids, onShowGridAreaHighlight } = this.props;

const {
  grids,
  ...
} = this.props;

::: devtools/client/inspector/layout/components/GridOutline.js:153
(Diff revision 1)
>    },
>  
>    onMouseOverCell({ target }) {
> -    // TODO: We will want onMouseOver interactions with the grid cells, which is addressed
> -    // in Bug 1337235
> +    const { grids, onShowGridAreaHighlight } = this.props;
> +    const nodeId = target.getAttribute("data-nodeId");
> +    const name = target.getAttribute("data-name");

Move this above nodeId

::: devtools/client/inspector/layout/layout.js:229
(Diff revision 1)
>  
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a change in the grid area being highlighted.

We first describe when this handler is called:
Handler for a mouseover in the grid outline of a grid area. Highlights the grid area in the CSS Grid Highlighter for the given grid area.

::: devtools/client/inspector/layout/layout.js:231
(Diff revision 1)
>        },
>  
>        /**
> +      * Handler for a change in the grid area being highlighted.
> +      * Shows the grid area currently being highlighted.
> +      * @param  {NodeFront} node

Format the JSDoc similar to what we have done before.

Comment 7

[Micah Tigley [:micah]]

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

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

Comment 8

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

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117004

::: devtools/client/inspector/layout/components/GridOutline.js:44
(Diff revision 4)
>        selectedGrids: grids.filter(grid => grid.highlighted),
>      });
>    },
>  
> -  renderGridCell(columnNumber, rowNumber, color) {
> +  /**
> +  * Returns the grid area name if the given grid cell is part of a grid area.

Each * needs 1 more space indentation. Should align with /*

::: devtools/client/inspector/layout/components/GridOutline.js:52
(Diff revision 4)
> +  *         The column number of the grid cell.
> +  * @param  {Number} row
> +  *         The row number of the grid cell.
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment
> +  * @returns  {String}

s/@returns/@return.

{String} should align with {Array} so remove one space. after @return.

::: devtools/client/inspector/layout/components/GridOutline.js:73
(Diff revision 4)
> +  *
> +  * @param  {Number} x
> +  *         The x-position of the grid cell.
> +  * @param  {Number} y
> +  *         The y-position of the grid cell.
> +  * @param  {Number} col

s/col/columnNumber

::: devtools/client/inspector/layout/components/GridOutline.js:75
(Diff revision 4)
> +  *         The x-position of the grid cell.
> +  * @param  {Number} y
> +  *         The y-position of the grid cell.
> +  * @param  {Number} col
> +  *         The column number of the grid cell.
> +  * @param  {Number} row

s/row/rowNumber

::: devtools/client/inspector/layout/components/GridOutline.js:78
(Diff revision 4)
> +  * @param  {Number} col
> +  *         The column number of the grid cell.
> +  * @param  {Number} row
> +  *         The row number of the grid cell.
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment

Add with a period.
s/List/Array
s/grid areas/grid areas data
s/on/in the

::: devtools/client/inspector/layout/components/GridOutline.js:82
(Diff revision 4)
> +  * @param  {Array} areas
> +  *         List of grid areas stored on grid fragment
> +  */
> +  renderGridCell(x, y, columnNumber, rowNumber, color, areas) {
> +    const name = this.getGridAreaName(columnNumber, rowNumber, areas);
>      return dom.rect(

I think we have a grid key that we can store in a data-id attribute to make getting the grid and its nodefront easier in onShowGridAreaHighlight so we don't have to traverse through the list of grids as well.

::: devtools/client/inspector/layout/components/GridOutline.js:85
(Diff revision 4)
> +  renderGridCell(x, y, columnNumber, rowNumber, color, areas) {
> +    const name = this.getGridAreaName(columnNumber, rowNumber, areas);
>      return dom.rect(
>        {
>          className: "grid-outline-cell",
> -        x: columnNumber,
> +        "data-name": name,

s/data-name/data-grid-area-name

::: devtools/client/inspector/layout/components/GridOutline.js:164
(Diff revision 4)
> +    const name = target.getAttribute("data-name");
> +    const color = target.getAttribute("stroke");
> +
> +    target.setAttribute("fill", color);
> +
> +    onShowGridAreaHighlight(name, color);

We need to make sure we aren't still highlighting the grid area when we leave a cell that has a grid area to a cell that doesn't have a grid area.

::: devtools/client/inspector/layout/layout.js:105
(Diff revision 4)
>          return this.swatchColorPickerTooltip;
>        },
>  
>        /**
>         * Set the inspector selection.
>         * @param {NodeFront} nodeFront

Add a new line before @param here while you are making fixes.

::: devtools/client/inspector/layout/layout.js:153
(Diff revision 4)
>  
>        onShowBoxModelEditor,
>        onShowBoxModelHighlighter,
>  
>       /**
>         * Shows the box-model highlighter on the element corresponding to the provided

Ah, I see the indentation for this is also messed up. We need to remove a space before the *. Can you fix this while we are at it.

::: devtools/client/inspector/layout/layout.js:167
(Diff revision 4)
>          let toolbox = this.inspector.toolbox;
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a mouseover in the grid outline of a grid area.

* needs 1 more space indented.

::: devtools/client/inspector/layout/layout.js:167
(Diff revision 4)
>          let toolbox = this.inspector.toolbox;
>          toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>        },
>  
>        /**
> +      * Handler for a mouseover in the grid outline of a grid area.

This isn't really a handler for any typical events. So, we should just be describing its functionality which is stated in the second sentence of the comments. We can remove this handler sentence.

Comment 9

[Micah Tigley [:micah]]

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

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

Comment 10

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

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117246

Comment 11

[Micah Tigley [:micah]]

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114762/diff/5-6/

Comment 12

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

Comment on attachment 8840275 [details]
Bug 1337235 - Mouseover interaction for grid outline.

https://reviewboard.mozilla.org/r/114762/#review117556

Comment 13

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

Attachment: bug_1337235_mouseover_interaction_for_grid_outline_r_gl

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a15dfecc9fa
Mouseover interaction for grid outline. r=gl

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a15dfecc9fa