1285745 - Split up toolbars.css into multiple files

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P3)

Description

[Tim Nguyen :ntim]

Suggested split:
- inputs.css (text, search, filter inputs)
- buttons.css (.devtools-button, .devtools-toolbarbutton, .devtools-menulist)
- tabs.css (.devtools-sidebar-tabs)
- toolbars.css (.devtools-toolbar, .devtools-separator)
- icons.css
- toolbox.css (for the command button CSS and the .devtools-tab CSS)

Documents will need to import each of those files based on their needs.

Why?:
- toolbars.css has became a CSS dump :D
- easier to maintain
- It seems like we started creating a CSS file per React component, and this follows well the trend
- Not all tools need everything that toolbars.css provides. For example, the DOM panel and the JSON viewer only need the toolbar button and search input styling, and they define their own:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/css/toolbar.css
https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/css/search-box.css

Questions:
- Where does `.devtools-throbber` go ?
- Should we drop the `devtools-` prefix from the class names? (though this belongs in a new bug)

Comment 1

[Tim Nguyen :ntim]

Brian, thoughts ?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #0)
> Suggested split:
> - inputs.css (text, search, filter inputs)
> - buttons.css (.devtools-button, .devtools-toolbarbutton, .devtools-menulist)
> - tabs.css (.devtools-sidebar-tabs)
> - toolbars.css (.devtools-toolbar, .devtools-separator)
> - icons.css
> - toolbox.css (for the command button CSS and the .devtools-tab CSS)
> 
> Documents will need to import each of those files based on their needs.

I'd rather have the theme control loading CSS files instead of each tool opting in to a subset of them.  I think tracing dependencies across tools adds extra work when debugging issues, since you need to track whether a file has been loaded in a tool at all.  It also decentralizes the imports which makes moving files around / renaming harder.

> Why?:
> - toolbars.css has became a CSS dump :D
> - easier to maintain

For those two, sure.  toolbars.css is a CSS dump that does more than toolbars.  I think we could rearrange some things - I was thinking we could put your proposed 'inputs', 'buttons', and 'icons' CSS into common.css, then make your proposed 'toolbox.css' and load it directly in toolbox.xul instead of in a shared file.  This would leave just the 'tabs' and 'toolbars' CSS files in toolbars.css, which seems right.

> - It seems like we started creating a CSS file per React component, and this
> follows well the trend
> - Not all tools need everything that toolbars.css provides. For example, the
> DOM panel and the JSON viewer only need the toolbar button and search input
> styling, and they define their own:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/css/
> toolbar.css
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/css/
> search-box.css

JSON viewer is a special case that isn't loaded in the toolbox so it's safest for it to use separate CSS anyway (to prevent unintended breakage when toolbox CSS / HTML is refactored).

> Questions:
> - Where does `.devtools-throbber` go ?

I'd move that into common.css.

> - Should we drop the `devtools-` prefix from the class names? (though this
> belongs in a new bug)

I like having it there for searchability across of all m-c.

Comment 3

[Tim Nguyen :ntim]

We want to implement bgrins' proposal in comment 2

Comment 4

[p.panayiotou2]

Hi. I am new and trying to split the files. I have a questions if someone could help. Should .menu-filter-button be moved as well in common.css?

Comment 5

[Tim Nguyen :ntim]

(In reply to p.panayiotou2 from comment #4)
> Hi. I am new and trying to split the files. I have a questions if someone
> could help. Should .menu-filter-button be moved as well in common.css?
Yeah, that belongs to the buttons category.

Comment 6

[p.panayiotou2]

Should I push to review branch(saw it here: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#submitting-commits-for-review) or add separate attachments to this bug for each file(common.css, toolbars.css, toolbox.css, toolbox.xul).
Thanks

Comment 7

[Tim Nguyen :ntim]

(In reply to p.panayiotou2 from comment #6)
> Should I push to review branch(saw it here:
> https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/
> commits.html#submitting-commits-for-review) or add separate attachments to
> this bug for each file(common.css, toolbars.css, toolbox.css, toolbox.xul).
> Thanks

You can submit it to mozreview, but I think you need permission to push.
I would recommend uploading your diff in one attachment on bugzilla.

Comment 8

[p.panayiotou2]

Attachment: Split of the css files as described

toolbox.css was already as described

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8783325 [details] [diff] [review]
Split of the css files as described

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

Your changes in toolbox.xul are not needed, toolbox.css is already imported by this file.

Brian, does this fit your proposal?

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8783325 [details] [diff] [review]
Split of the css files as described

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

Yes, this is just what I had in mind - thanks!  Can you please format the patch with a commit message and author information so we can land it: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F.

::: devtools/client/framework/toolbox.xul
@@ +5,4 @@
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>  <?xml-stylesheet href="chrome://devtools/skin/toolbox.css" type="text/css"?>
>  <?xml-stylesheet href="resource://devtools/client/shared/components/notification-box.css" type="text/css"?>
> +<?xml-stylesheet href="resource://devtools/client/themes/toolbox.css" type="text/css"?>

This new stylesheet link should be removed

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8783325 [details] [diff] [review]
Split of the css files as described

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

r=me with those and the commit message addressed.

::: devtools/client/framework/toolbox.xul
@@ +5,4 @@
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>  <?xml-stylesheet href="chrome://devtools/skin/toolbox.css" type="text/css"?>
>  <?xml-stylesheet href="resource://devtools/client/shared/components/notification-box.css" type="text/css"?>
> +<?xml-stylesheet href="resource://devtools/client/themes/toolbox.css" type="text/css"?>

Please remove the new stylesheet link.

::: devtools/client/themes/common.css
@@ +319,5 @@
> +}
> +
> +/* Set flex attribute to Toolbox buttons and Picker container so,
> +   they don't overlapp with the tab bar */
> +#toolbox-buttons {

The toolbox-buttons stuff should be in toolbox.css

@@ +323,5 @@
> +#toolbox-buttons {
> +  display: flex;
> +}
> +
> +#toolbox-picker-container {

Same here.

Comment 12

[p.panayiotou2]

Attachment: Split of css files with suggested changes

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8783859 [details] [diff] [review]
Split of css files with suggested changes

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

::: devtools/client/themes/common.css
@@ +676,5 @@
> +/*
> + * Filter buttons
> + * @TODO : Fix when https://bugzilla.mozilla.org/show_bug.cgi?id=1255116 lands
> + */
> +.menu-filter-button {

Can you move filter-button related rules right after the button related ones ?

::: devtools/client/themes/toolbox.css
@@ +401,5 @@
> +#toolbox-buttons {
> +  display: flex;
> +}
> +
> +#toolbox-picker-container {

Can you move both of these rules (#toolbox-picker-container and #toolbox-buttons) after the #toolbox-tabs rule on line 54 of this file ?

Comment 14

[p.panayiotou2]

Attachment: Code rearrangement in the files split

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8783896 [details] [diff] [review]
Code rearrangement in the files split

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

Seems like this patch applies on top on the previous one, can you fold them together ?
See http://codefirefox.com/video/move-fold-patch on how to fold them.

Comment 16

[p.panayiotou2]

They are already folded aren't they? I had popped my old patch before making the changes.

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8783896 [details] [diff] [review]
Code rearrangement in the files split

They are folded, I must have clicked diff instead of review by mistake. Sorry for the trouble.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/93e88642653d
Split toolbar.css into multiple files. r=ntim

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93e88642653d