Closed Bug 1597093 Opened 4 months ago Closed 4 months ago

use shared css files for EditorDialog.css and remove editor.css

Categories

(Thunderbird :: Theme, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(2 files, 1 obsolete file)

A bunch of css files have platform specific css files, but the rules are pretty much the same. Should use a shared css file to ease maintenance. E.g. editor.css

Turns out editor.css is same across all platforms, but just junk (except for that it's importing the base skin)

Attachment #9109312 - Flags: review?(richard.marti)

EditorDialog.css

Attachment #9109313 - Flags: review?(richard.marti)
Comment on attachment 9109312 [details] [diff] [review]
bug1597093_shared_editor_css.patch

LGTM
Attachment #9109312 - Flags: review?(richard.marti) → review+
Comment on attachment 9109313 [details] [diff] [review]
bug1597093_shared_EditorDialog_css.patch

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

r+ with the comments addressed.

::: mail/themes/linux/editor/EditorDialog.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +@import url("chrome://messenger/skin/shared/EditorDialog.css");

Please add a empty line after the @import line.

@@ +6,3 @@
>  groupbox {
> +  margin: 5px;
> +  padding-top: 2px;

This line has no effect as the following padding overwrites the value.

@@ +11,1 @@
>  .spell-check {

Please a empty line between the rules.

::: mail/themes/osx/editor/EditorDialog.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +@import url("chrome://messenger/skin/shared/EditorDialog.css");

Please add a empty line after the @import line.

::: mail/themes/shared/mail/EditorDialog.css
@@ +129,3 @@
>    margin-bottom: 2px;
>    margin-inline-start: 4px;
>    margin-inline-end: 2px;

This four lines could be written:
margin-block: 1px 2px;
margin-inline: 4px 2px;
I could do this clean up in a follow-up bug.

@@ +289,1 @@
>    overflow   : -moz-hidden-unscrollable;

This file has a lot of weird spaces. Do you remove them in this bug or do you want that I clean them in a follow-up bug?
Attachment #9109313 - Flags: review?(richard.marti) → review+

Addressed the review comments, and took care of the bad whitespaces (+ removed an unused .tb-margin rule)

Attachment #9109313 - Attachment is obsolete: true
Attachment #9109363 - Flags: review+
Status: NEW → ASSIGNED
Summary: use shared css files for editor.css and some others → use shared css files for EditorDialog.css and remove editor.css

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d207eeb0cef0
remove editor.css. r=Paenglab
https://hg.mozilla.org/comm-central/rev/6e169e7759f7
use shared css files for EditorDialog.css. r=Paenglab DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
You need to log in before you can comment on or make changes to this bug.