Split up toolbars.css into multiple files

RESOLVED FIXED in Firefox 51

Status

P3
enhancement
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: ntim, Assigned: p.panayiotou2)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
Firefox 51
good-first-bug
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [good taipei bug][lang=css])

Attachments

(1 attachment, 2 obsolete attachments)

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)
Brian, thoughts ?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
Severity: normal → enhancement
Depends on: 1286872
Priority: -- → P3
We want to implement bgrins' proposal in comment 2
Keywords: good-first-bug
Whiteboard: [good taipei bug][lang=css]
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Updated

2 years ago
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 6

2 years ago
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
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8783325 [details] [diff] [review]
Split of the css files as described

toolbox.css was already as described
Attachment #8783325 - Flags: review?(ntim.bugs)
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?
Attachment #8783325 - Flags: review?(ntim.bugs) → review?(bgrinstead)
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
Attachment #8783325 - Flags: review?(bgrinstead)
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.
Attachment #8783325 - Flags: review+
(Assignee)

Comment 12

2 years ago
Created attachment 8783859 [details] [diff] [review]
Split of css files with suggested changes
Attachment #8783859 - Flags: review?(ntim.bugs)
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 ?
Attachment #8783859 - Flags: review?(ntim.bugs) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8783896 [details] [diff] [review]
Code rearrangement in the files split
Attachment #8783896 - Flags: review?(ntim.bugs)
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.
Attachment #8783896 - Flags: review?(ntim.bugs)
(Assignee)

Comment 16

2 years ago
They are already folded aren't they? I had popped my old patch before making the changes.
Flags: needinfo?(ntim.bugs)
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.
Flags: needinfo?(ntim.bugs)
Attachment #8783896 - Flags: review+
Attachment #8783859 - Attachment is obsolete: true
Attachment #8783325 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → p.panayiotou2

Comment 18

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/93e88642653d
Split toolbar.css into multiple files. r=ntim
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93e88642653d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.