1343777 - Display grid cell highlights on mouseover events for grid outline

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Micah Tigley [:micah]]

Mouseover events on the grid outline should also display grid cell highlights for the grid cell being hovered over.

Comment 1

[Micah Tigley [:micah]]

Attachment: Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

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

Comment 2

[Micah Tigley [:micah]]

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

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

Comment 3

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

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

https://reviewboard.mozilla.org/r/116516/#review118778

::: devtools/client/inspector/grids/components/Grid.js:65
(Diff revision 2)
>          },
>          showGridOutline ?
>            GridOutline({
>              grids,
>              onShowGridAreaHighlight,
> +            onShowGridCellHighlight

Add , to the end

::: devtools/client/inspector/grids/components/GridOutline.js:173
(Diff revision 2)
>  
>    onMouseLeaveCell({ target }) {
>      const {
>        grids,
>        onShowGridAreaHighlight,
> +      onShowGridCellHighlight

Add a , to the end

Comment 4

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

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

https://reviewboard.mozilla.org/r/116516/#review118812

Comment 5

[Micah Tigley [:micah]]

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

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

Comment 6

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

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

https://reviewboard.mozilla.org/r/116516/#review119290

::: devtools/client/inspector/grids/components/GridOutline.js:77
(Diff revision 3)
>     * @param  {Object} grid
>     *         A single grid container in the document.
> +   * @param  {Number} gridFragmentIndex
> +   *         The index of the grid fragment rendered to the document.
>     */
> -  renderGrid(grid) {
> +  renderGrid(grid, gridFragmentIndex) {

We don't know the gridFragmentIndex until we actually render the grid. You can see in line 81 the gridFragment we are rendering is index = 0, and we only render the first grid fragment of the grid container. 

Usually we would have a for loop that loops over all the grid fragments and render each one individually.

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

you can declare gridFragmentIndex = 0; before this line.

::: devtools/client/inspector/grids/components/GridOutline.js:158
(Diff revision 3)
>    renderGridOutline(grids) {
>      return dom.g(
>        {
>          className: "grid-cell-group",
>        },
> -      grids.map(grid => this.renderGrid(grid))
> +      grids.map((grid, index) => this.renderGrid(grid, index))

This should continue to be:
grids.map(grid => this.renderGrid(grid))

::: devtools/client/inspector/grids/components/GridOutline.js:187
(Diff revision 3)
>      const color = target.getAttribute("stroke");
>  
>      target.setAttribute("fill", "none");
>  
>      onShowGridAreaHighlight(grids[id].nodeFront, null, color);
> +    onShowGridCellHighlight(id, 0, 0);

We seems to have the wrong onShowGridCellHighlight signature here since it actually takes 4 arguments instead of 3.

We still need to pass in the nodeFront. We need to grab the fragmentIndex here since the grid-id is not the grid-fragment-index. 

Instead of 0, I would use null.

::: devtools/client/inspector/grids/components/GridOutline.js:194
(Diff revision 3)
>  
>    onMouseOverCell({ target }) {
>      const {
>        grids,
>        onShowGridAreaHighlight,
> +      onShowGridCellHighlight

Add a , to the end.

::: devtools/client/inspector/grids/components/GridOutline.js:209
(Diff revision 3)
>  
> +    if (name) {
> -    onShowGridAreaHighlight(grids[id].nodeFront, name, color);
> +      onShowGridAreaHighlight(grids[id].nodeFront, name, color);
> +    }
> +
> +    onShowGridCellHighlight(grids[id].nodeFront, fragmentIndex, rowNumber, columnNumber);

We should have a if statement check to make sure we have a valid fragmentIndex, rowNumber and columnNumber.

::: devtools/client/inspector/grids/grid-inspector.js:347
(Diff revision 3)
> +   *         The column number of the grid cell for which the grid highlighter
> +   *         is highlighted for.
> +   */
> +  onShowGridCellHighlight(node, gridFragmentIndex, rowNumber, columnNumber) {
> +    let { highlighterSettings } = this.store.getState();
> +    highlighterSettings.showGridCell = {gridFragmentIndex: -1, rowNumber, columnNumber};

We shouldn't need to have this default. onShowGriCellHighlight call should assume we have a valid grid fragment index, row and column number that we want to highlight.

Comment 7

[Micah Tigley [:micah]]

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

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

Comment 8

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

Comment on attachment 8842755 [details]
Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.

https://reviewboard.mozilla.org/r/116516/#review119690

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/847d96177f53
Display grid cell highlights on mouseover events for grid outline. r=gl

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/847d96177f53