Closed
Bug 947657
Opened 10 years ago
Closed 10 years ago
[Shared Themes] Pt. 1 - Move messenger.css code to shared/
Categories
(Thunderbird :: Theme, enhancement)
Thunderbird
Theme
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 28.0
People
(Reporter: jsbruner, Assigned: jsbruner)
References
Details
Attachments
(1 file, 2 obsolete files)
19.18 KB,
patch
|
jsbruner
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Here's the patch that: A. Creates a shared directory. So we now have themes/linux, themes/osx/, themes/windows/, and themes/shared. B. Moves shared code from all platforms and adds them to themes/shared/mail/messenger.inc.css. Richard, I have only tested this on OS X, (Linux builds are failing for me, and my Windows VM isn't functioning properly). I am trying to fix the Linux builds, so if I find there is an issue before you do I'll clear the review flags. In addition, I am currently using %include's to import the proper style sheets, and although this works fine, it has the disadvantage of requiring a re-build (Files must get pre-processed to import the CSS). Although I don't really care, I would like to hear your thoughts on that issue. And thanks so much for reviewing this!
Attachment #8344249 -
Flags: review?(richard.marti)
Comment 2•10 years ago
|
||
Comment on attachment 8344249 [details] [diff] [review] Patch. Review of attachment 8344249 [details] [diff] [review]: ----------------------------------------------------------------- This looks already good. But needs some changes to become a r+. ::: mail/themes/linux/mail/messenger.css @@ +12,2 @@ > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console. @@ +15,5 @@ > toolbar:not(.inline-toolbar):not(:-moz-lwtheme) { > -moz-appearance: menubar; > } > > #tabbar-toolbar { This and #tabbar-toolbar[customizing="true"] are in messenger.inc.css and you can remove it from here. @@ +369,4 @@ > > notification[value="addon-install-failed"] .messageCloseButton { > -moz-appearance: none; > list-style-image: url("chrome://messenger/skin/icons/close-inverted.png"); You can remove the list-style-image. It's defined in messenger.inc.css. ::: mail/themes/osx/mail/messenger.css @@ +12,2 @@ > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console. @@ +12,4 @@ > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > > +#tabbar-toolbar { Why are you adding this? This is already in messenger.inc.css. @@ +38,5 @@ > /* :::::: throbber :::::::::: */ > > #throbber-box { > + width: 17px !important; > + margin: 0 4px !important; Are both !important needed? This comes after the messenger.inc.css definition and should oversteer it without !important. ::: mail/themes/shared/mail/messenger.inc.css @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public With a %if 0 /*...*/ %endif around this comment it isn't added to the output file. I think one time in the file is enough :) @@ +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/. */ > + > +/* ===== messenger.inc.css ================================================== I think this comment isn't needed in this file. One time in messenger.css is enough. @@ +71,5 @@ > +notification[value="addon-install-failed"] { > + background-image: linear-gradient(rgb(255, 100, 100), rgb(204, 85, 85)); > +} > + > +notification[value="addon-install-complete"] { This is a OS X only rule and doesn't fit to the other themes. Please leave it there. @@ +82,5 @@ > +notification[value="addon-install-complete"] .notification-inner { > + border: none; > +} > + > +notification[value="addon-install-blocked"] > button, The notification > button rules are OS X only and are making the buttons on the other platforms bigger. Please let all this rules in osx. ::: mail/themes/windows/mail/messenger-aero.css @@ +2,5 @@ > * 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/. */ > > %include messenger.css > +%include ../../shared/mail/messenger.inc.css No need to include it here. It's already included in messenger.css which is added the line above. ::: mail/themes/windows/mail/messenger.css @@ +12,2 @@ > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); Please remove the namespace line. It's already defined in messenger.inc.css and gives a error in console.
Attachment #8344249 -
Flags: review?(richard.marti) → review-
Assignee | ||
Comment 3•10 years ago
|
||
Addresses review feedback.
Attachment #8344249 -
Attachment is obsolete: true
Attachment #8344370 -
Flags: review?(richard.marti)
Comment 4•10 years ago
|
||
Comment on attachment 8344370 [details] [diff] [review] Patch. Review of attachment 8344370 [details] [diff] [review]: ----------------------------------------------------------------- Yeah great. Sorry to not mention the two comments in previous patch. With this fixed r=me ::: mail/themes/linux/mail/messenger.css @@ +50,3 @@ > /* ::::: throbber ::::: */ > > #throbber-box { The throbber-box can be removed, it is already in messenger.inc.css. ::: mail/themes/osx/mail/messenger.css @@ +13,5 @@ > #titlebar { > -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox"); > height: 22px; > + > + /* Until we draw tabs in the titlebar on OS X we must Thank you for removing the whitespaces :) ::: mail/themes/windows/mail/messenger.css @@ +41,3 @@ > /* ::::: throbber ::::: */ > > #throbber-box { The throbber-box can be removed, it is already in messenger.inc.css.
Attachment #8344370 -
Flags: review?(richard.marti) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks Richard! Running through try before landing... https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=0a9970f7b8b9
Attachment #8344370 -
Attachment is obsolete: true
Attachment #8344378 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/0d59aed637f9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
You need to log in
before you can comment on or make changes to this bug.
Description
•