1498115 - Align the flexbox layout sidebar with the latest mockup

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Attachment: image (1).png

As seen in the attached new mockup, there are a number of small changes we'd like to make the flexbox inspector so it's easier to use:

- Add a "Flex Items" header before the list of items
- Highlight the corresponding item in the page when hovering over an item in the list (I think this is actually bug 1495775)
- Add a "Overlay" label next to the highlighter toggle so people understand what it is for
- Remove the Flexbox container properties section at the bottom, and replace them with little badges next to the flex container Rep for: direction, wrap and alignment.
Although I would note that we have a big project coming up for alignment later, so that last badge doesn't necessarily have to land.

Comment 1

[Victoria Wang [:victoria]]

Thanks! I had trouble loading Invision yesterday but here's the link in case you want to see it in context with the other mockups. https://mozilla.invisionapp.com/share/MZNA228P5WB#/screens/319386528

Comment 2

[Victoria Wang [:victoria]]

gl: In case it helps here's the inspect link https://mozilla.invisionapp.com/d/#/console/14988591/319386528/inspect

Comment 3

[Patrick Brosset <:pbro>]

Attachment: image.png

2 more things that aren't currently in line with the latest mockups: 
- hovering over flex items in the list of items should highlight the whole row, not just the element Rep
- larger/bolder numbers for flex items

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #3)
> - hovering over flex items in the list of items should highlight the whole
> row, not just the element Rep
I should also note that clicking anywhere on the row should open the flex item sizing panel, not just when you click on the Rep.

Comment 5

[Victoria Wang [:victoria]]

A couple polish items from https://discourse.mozilla.org/t/flexbox-inspector-input-wanted/30611/18?

- would be great if the width of the sibling dropdown could expand larger to fit larger element names, e.g. large as 90% of the space - but still centered with a left-aligned arrows icon :D

- Non overlapping labels but I'm sure you're already aware of this :)

Comment 6

[Victoria Wang [:victoria]]

One more thing I wanted to mention, based on me navigating around between items and containers:

When viewing a flex item accordion, if we won't be able to overlay the sizing diagram in-page like this: https://mozilla.invisionapp.com/share/MZNA228P5WB#/screens/311896910

I feel we should display a basic highlight over the item in-page like this: https://mozilla.invisionapp.com/share/MZNA228P5WB#/screens/319386528

Currently it seems easy to lose the context of where the item is in the page when drilling down into an item.

Comment 7

[Victoria Wang [:victoria]]

Also, a polish item for the accordion headers that start with "Flex item of..." - truncate long headers so they only take up one line.

(new.reddit.com has a selector named header._2GyPfdsi-MbQFyHRECo9GO.cx1ohrUAq6ARaXTX2u8YN .aswboo-0.isJDXQ.s1czsxkv-0.bYsyvY)

Comment 8

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

Attachment: Part 1: Remove the FlexboxContainerProperties component. [1.0]

Comment 9

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

Attachment: Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [1.0]

Comment 10

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

Attachment: Part 3: Implement the new styles for the flex item list.

Comment 11

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

We decided to hold off on adding the "Overlay" text until Victoria determines if this is useful or if we can keep it the same from her UX research. that is being conducted this week.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 9023082 [details] [diff] [review]
Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [1.0]

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

::: devtools/client/inspector/markup/views/element-editor.js
@@ +286,5 @@
>    },
>  
>    _createEventBadge: function() {
>      this._eventBadge = this.doc.createElement("div");
> +    this._eventBadge.className = "markup-badge inspector-badge";

These 2 class names feel a bit redundant now. Could we live with just 1 class? inspector-badge? (and therefore rename all the --markup-badge variables to --inspector-badge)

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 9023082 [details] [diff] [review]
Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [1.0]

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

One thing I forgot to note about this patch too is: I'd like to see at least one test that checks the existence of these badges in the DOM.
And perhaps another one that checks their correctness (something that iterates over a few simple test cases with/without wrap and with different directions and checks the values displayed in the badges)

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 9023083 [details] [diff] [review]
Part 3: Implement the new styles for the flex item list.

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

Code changes look good. And this is a good improvement.
There are still a few minor things to polish up before we can land this:

- The Flex Items label is not vertically aligned with Flex container Rep above it (and its badges). It's a few pixels to the left.
- The item's numbers appear bigger and bolder on Victoria's mockup
- The right arrowhead > to the right of each item is too close to the edge of the panel. In the mockups, there's a bit more spacing here.

Comment 15

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

Attachment: Part 1: Remove the FlexboxContainerProperties component. [2.0]

Comment 16

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

Attachment: Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [2.0]

Comment 17

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

Attachment: Part 3: Implement the new styles for the flex item list. [2.0]

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 9024914 [details] [diff] [review]
Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [2.0]

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

Thanks, this looks good to me. I only have 2 remaining questions/comments and I'd like a chance to re-review once answered/addressed:

- would it make sense to create a new badge.css file for an even smaller import size?
- see my comment below, it seems to me like we can clean some variables even more.

::: devtools/client/themes/markup.css
@@ +6,5 @@
>    --markup-badge-border-color: #CACAD1;
>    --markup-badge-color: var(--grey-60);
>    --markup-badge-hover-background-color: #DFDFE8;
>    --markup-badge-interactive-background-color: var(--grey-20);
>    --markup-badge-interactive-color: var(--grey-90);

Why did you keep these variables here. Since we're moving the badge styling to inspector.css, it seems like these variables should be deleted too.

Also, I can see that you've kept on using these specific markup-badge variables in this file, for the markup-expand-badge elements. But I don't understand why we can't use the inspector-badge variable there too now.

Comment 19

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

Attachment: Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [3.0]

Comment 20

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

Attachment: Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. [4.0]

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b4d9761d46
Part 1: Remove the FlexboxContainerProperties component. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad62c3439651
Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f8e9c7301b
Part 3: Implement the new styles for the flex item list. r=pbro

Comment 22

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1498115) for failures in browser_flexbox_container_properties.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=e7f8e9c7301bc4cd4088bcf244ca55b877fbb034&selectedJob=211993244

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211993244&repo=mozilla-inbound&lineNumber=4722

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4228a1200fdc057b021a1bd6a2fcd654c37b5c4d

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df843a2c7c75
Part 1: Remove the FlexboxContainerProperties component. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbcd0df86411
Part 2: Add badges to show the flex-direction and flex-wrap properties of the flex container. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0051c8d339a9
Part 3: Implement the new styles for the flex item list. r=pbro

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/df843a2c7c75
https://hg.mozilla.org/mozilla-central/rev/bbcd0df86411
https://hg.mozilla.org/mozilla-central/rev/0051c8d339a9