1577536 - Add a --theme-toolbar-tall-height variable for the 28px DevTools toolbars

Status: NEW

(DevTools :: Shared Components, enhancement)

Description

[Florens Verschelde :fvsch]

Console and Network define their toolbar heights directly, overriding .devtools-toolbar.

Create a variable (--theme-toolbar-tall-height: 28px) and a modifier class (.devtools-toolbar-tall) for the taller toolbars, and apply them to Console and Network.

Comment 1

[Florens Verschelde :fvsch]

Attachment: Bug 1577536 - Move toolbar styles to toolbars.css and add tall variant; r=ladybenko

Comment 2

[Florens Verschelde :fvsch]

Attachment: Bug 1577536 - Use devtools-tall-toolbar in Netmonitor; r=Honza

Depends on D63839

Comment 3

[Florens Verschelde :fvsch]

Attachment: Bug 1577536 - Use devtools-tall-toolbar in Console; r=nchevobbe

Depends on D63840

Comment 4

[Florens Verschelde :fvsch]

After discussion in https://phabricator.services.mozilla.com/D63839 I moved to using the "inclusive height" for toolbar height variables:

/* Toolbar size: 24px and 28px, plus 1px for the toolbar's border */
--theme-toolbar-height: 25px;
--theme-toolbar-tall-height: 29px;

This lets us use var(--theme-toolbar-height) directly in most settings, and only rely on calc expressions like calc(var(--theme-toolbar-height) - 1px) exceptionally.

My original take was that it was more straightforward to define the content height and use calc to add the border height only when needed, but "only when needed" proved to be 90% of the time.

Comment 5

[Florens Verschelde :fvsch]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=292218098&revision=83c0f023b6dde321c082b3da6b7e1a55fceabffd

Comment 6

[Florens Verschelde :fvsch]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4673372aac92f5dc3668991d73e5d5bf3be9953

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:fvsch, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Florens Verschelde :fvsch]

Hi Belén, would you have time to review D63839 again?
After your first review and feedback from Tim as well, I changed how the --theme-toolbar-height variables are defined as described here: https://phabricator.services.mozilla.com/D63839#1998970

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:Honza, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas, should we use the patch(es)?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #10)

Nicolas, should we use the patch(es)?

This wasn't a trivial change, and the patch were created a few years back, so I wouldn't apply them as is.
We could reuse part of it/take inspiration from it if we ever want to do this work, but I don't think this is a high priority at the moment