1249555 - Display edge of explicit grid and implicit grid differently in the css grid inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

The concept of implicit grid is key in css grids. Depending on how you position elements in the grid, implicit lines may be created automatically to fit them on.

I imagine this concept can be pretty hard to understand at the start and may surprise people.

If the css grid overlay we intend to build in bug 1181227 would colorize implicit lines differently, this would help a lot.

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cbf5bdf4f4
Changes to the ARM simulator; r=sunfish

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Pulsebot from comment #1)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/08cbf5bdf4f4
> Changes to the ARM simulator; r=sunfish

Sorry this was backed out (wrong bug number) in:

o  changeset:   307854:2dda5f625bc0
|  summary:     Backed out changeset 3e7e914abf3d for landing with the wrong bug number;

Comment 3

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

Attachment: 1249555.patch [1.0]

Comment 4

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

Attachment: 1249555.patch [2.0]

Comment 5

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

Bug 1289200 actually adds the grid line types which will help identify implicit and explicit grid lines. I would expect the approach to figuring out explicit/implicit grid to be different when that lands. For the time being, I have used GridTracks to figure out the grid line type.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8780631 [details] [diff] [review]
1249555.patch [2.0]

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

Code looks good, I only have a few comments to make it a bit shorter and easier to understand.
Other than this, the only blockers I see here are:
- Can you explain the reasoning behind showing the edges of the explicit grid differently than other lines?
We already have a distinction between implicit and explicit lines, isn't that enough for users? I haven't had enough experience with grids so far to judge if this would be a useful information to display.
- See my question below about colors and styles.

Once these are cleared, I can R+.

::: devtools/server/actors/highlighters/css-grid.js
@@ +13,5 @@
>  const LINE_PADDING = 10;
> +const GRID_LINES_PROPERTIES = {
> +  "edge": {
> +    lineDash: [0, 0],
> +    strokeStyle: "#4B0082"

Do these dash styles and colors come from Helen's mockups? Just want to make sure we're not coming up with these ourselves and have some design guidance here.
The colors do seem very very close and when they're applied to 1px-wide lines, it's hard to see the color difference. The different dash styles do help though.

@@ +168,5 @@
>      return fragment.cols.lines[fragment.cols.lines.length - 1].start;
>    },
>  
> +  getLastEdgeLineIndex(tracks) {
> +    let lineIndex = tracks.length - 1;

The logic in this function through me off for a while because of the name of this variable.
This variable really is a trackIndex, not a lineIndex.
With this name, the logic becomes easier to follow. You're just going through tracks, from the last one, in reverse direction, until you find one that is implicit, and then you return the index of the line after this track.

@@ +206,5 @@
>        }
>      }
>    },
>  
>    renderRowLines(rows, {bounds}, startColPos, endColPos) {

renderRowLines and renderColLines are starting to get big, and they share essentially all of the same code, except that one is for vertical lines and the other for horizontal lines.
So I think we should have a meta renderLines function that takes a few more arguments like mainSide ("top" for rows and "left" for cols), crossSide ("left" for rows and "top" for cols), mainSize ("width" for rows and "height" for cols), crossSize ...
This way we can have just one renderLines function that has all of this code in common.
I've used the main/cross vocabulary from flexbox.

@@ +235,5 @@
>        }
>      }
>    },
>  
> +  renderLine(x1, y1, x2, y2, gridType) {

The variable gridType might be better named as lineType.

Comment 7

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

Attachment: 1249555.patch [3.0]

Comment 8

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

(In reply to Patrick Brosset <:pbro> from comment #6)
> Comment on attachment 8780631 [details] [diff] [review]
> 1249555.patch [2.0]
> 
> Review of attachment 8780631 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code looks good, I only have a few comments to make it a bit shorter and
> easier to understand.
> Other than this, the only blockers I see here are:
> - Can you explain the reasoning behind showing the edges of the explicit
> grid differently than other lines?
> We already have a distinction between implicit and explicit lines, isn't
> that enough for users? I haven't had enough experience with grids so far to
> judge if this would be a useful information to display.
> - See my question below about colors and styles.
> 
> Once these are cleared, I can R+.

We already talked about this, but this is based on Helen's design. Colors were provided by Helen. The line dash numbers were something I came up with based on Helen's design, but it should probably be verify by Helen. Let's do this in a follow up in a week after Helen is done with her UX analysis.
 
> 
> ::: devtools/server/actors/highlighters/css-grid.js
> @@ +13,5 @@
> >  const LINE_PADDING = 10;
> > +const GRID_LINES_PROPERTIES = {
> > +  "edge": {
> > +    lineDash: [0, 0],
> > +    strokeStyle: "#4B0082"
> 
> Do these dash styles and colors come from Helen's mockups? Just want to make
> sure we're not coming up with these ourselves and have some design guidance
> here.
> The colors do seem very very close and when they're applied to 1px-wide
> lines, it's hard to see the color difference. The different dash styles do
> help though.
> 

See above.

> @@ +168,5 @@
> >      return fragment.cols.lines[fragment.cols.lines.length - 1].start;
> >    },
> >  
> > +  getLastEdgeLineIndex(tracks) {
> > +    let lineIndex = tracks.length - 1;
> 
> The logic in this function through me off for a while because of the name of
> this variable.
> This variable really is a trackIndex, not a lineIndex.
> With this name, the logic becomes easier to follow. You're just going
> through tracks, from the last one, in reverse direction, until you find one
> that is implicit, and then you return the index of the line after this track.
> 

Fixed.

> @@ +206,5 @@
> >        }
> >      }
> >    },
> >  
> >    renderRowLines(rows, {bounds}, startColPos, endColPos) {
> 
> renderRowLines and renderColLines are starting to get big, and they share
> essentially all of the same code, except that one is for vertical lines and
> the other for horizontal lines.
> So I think we should have a meta renderLines function that takes a few more
> arguments like mainSide ("top" for rows and "left" for cols), crossSide
> ("left" for rows and "top" for cols), mainSize ("width" for rows and
> "height" for cols), crossSize ...
> This way we can have just one renderLines function that has all of this code
> in common.
> I've used the main/cross vocabulary from flexbox.
> 

Fixed.

> @@ +235,5 @@
> >        }
> >      }
> >    },
> >  
> > +  renderLine(x1, y1, x2, y2, gridType) {
> 
> The variable gridType might be better named as lineType.

Fixed.

Comment 9

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

Current patch doesn't rely on Bug 1289200.

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/8173084bb8ebc4695a130e47575c8ff024d3f896
Bug 1249555 - Display edge of explicit grid and implicit grid differently in the css grid inspector r=pbro

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8173084bb8eb