Align the flexbox layout sidebar with the latest mockup

RESOLVED FIXED in Firefox 65

Status

enhancement
P2
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: pbro, Assigned: gl)

Tracking

(Blocks 1 bug)

unspecified
Firefox 65
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

472.87 KB, image/png
Details
90.25 KB, image/png
Details
7.01 KB, patch
Details | Diff | Splinter Review
7.99 KB, patch
pbro
: review+
Details | Diff | Splinter Review
38.96 KB, patch
pbro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 months ago
Posted image 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.
(Assignee)

Updated

6 months ago
Blocks: dt-flexbox
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
(Assignee)

Updated

6 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Assignee: gl → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

6 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
(Reporter)

Comment 3

6 months ago
Posted image 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
(Reporter)

Comment 4

6 months ago
(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.
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 :)
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.
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)
(Assignee)

Comment 8

6 months ago
Attachment #9023081 - Flags: review?(pbrosset)
(Assignee)

Comment 10

6 months ago
Attachment #9023083 - Flags: review?(pbrosset)
(Assignee)

Comment 11

6 months ago
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.
(Reporter)

Updated

5 months ago
Attachment #9023081 - Flags: review?(pbrosset) → review+
(Reporter)

Comment 12

5 months ago
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)
Attachment #9023082 - Flags: review?(pbrosset)
(Reporter)

Comment 13

5 months ago
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)
(Reporter)

Comment 14

5 months ago
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.
Attachment #9023083 - Flags: review?(pbrosset)
(Assignee)

Comment 17

5 months ago
Attachment #9023083 - Attachment is obsolete: true
Attachment #9024915 - Flags: review?(pbrosset)
(Reporter)

Comment 18

5 months ago
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.
Attachment #9024914 - Flags: review?(pbrosset)
(Reporter)

Updated

5 months ago
Attachment #9024915 - Flags: review?(pbrosset) → review+
(Reporter)

Updated

5 months ago
Attachment #9025137 - Flags: review?(pbrosset) → review+

Comment 21

5 months ago
Pushed by gabriel.luong@gmail.com:
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
(Assignee)

Updated

5 months ago
Flags: needinfo?(gl)

Comment 23

5 months ago
Pushed by gabriel.luong@gmail.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df843a2c7c75
https://hg.mozilla.org/mozilla-central/rev/bbcd0df86411
https://hg.mozilla.org/mozilla-central/rev/0051c8d339a9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Blocks: 1507844
You need to log in before you can comment on or make changes to this bug.