Closed Bug 1343777 Opened 7 years ago Closed 7 years ago

Display grid cell highlights on mouseover events for grid outline

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: micah, Assigned: micah)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1343777 - Display grid cell highlights on mouseover events for grid outline.
59 bytes, text/x-review-board-request
gl
: review+
Details 
Mouseover events on the grid outline should also display grid cell highlights for the grid cell being hovered over.
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #8842755 - Flags: review?(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
Attachment #8842755 - Flags: review?(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
Attachment #8842755 - Flags: review?(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.
Attachment #8842755 - Flags: review?(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
Attachment #8842755 - Flags: review?(gl) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/847d96177f53
Display grid cell highlights on mouseover events for grid outline. r=gl
https://hg.mozilla.org/mozilla-central/rev/847d96177f53
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: