Closed Bug 953011 Opened 10 years ago Closed 10 years ago

[Shared Themes] Pt. 2 - Move mailWindow1.css code to shared/

Categories

(Thunderbird :: Theme, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: jsbruner, Assigned: jsbruner)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Move code to shared/ (obsolete) — Splinter Review
Summary says it all.

And, since it's Christmas time, I decided to give Richard a present. ;)

Here, have a review.
Attachment #8351294 - Flags: review?(richard.marti)
Comment on attachment 8351294 [details] [diff] [review]
Move code to shared/

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

::: mail/themes/shared/mail/mailWindow1.inc.css
@@ +3,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/.
> +*/
> +#endif

This is wrong in a few ways. 
It should be %if 0 + %endif, "#" in the licence block should be " *" .

But on the other hand, css allows you to have comment blocks with /* */ so i don't know if the preprocessing is needed.
Comment on attachment 8351294 [details] [diff] [review]
Move code to shared/

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

You forgot to write "Don't open before Christmas" ;)

r+ with the comments from Magnus and me addressed.

One question I had by creating my bug 952953: Do we need the mail directory in shared? Couldn't we place the files directly in shared? Naturally shared files from addrbook, compose etc. will be placed in their subdirectories. Now in the early state of shared files we could adapt our patches and only for messenger.inc.css a tiny patch would be needed.

::: mail/themes/osx/mail/mailWindow1.css
@@ +38,3 @@
>    color: -moz-DialogText;
>    /* Draw the top border of the tabstrip when core doesn't do it for us: */
>    border-top: 1px solid hsla(0, 0%, 0%, 0.3);

This two lines are coming from my patch from bug 952953. Your patch would fail applying on a tree without my patch.

@@ -609,5 @@
> -#quotaMeter {
> -  -moz-appearance: none;
> -  min-width: 4em;
> -  max-width: 4em;
> -  border: 1px solid #8B8B8B;

On shared file the color is ThreeDShadow. Does this look good on OS X?

::: mail/themes/shared/mail/mailWindow1.inc.css
@@ +54,5 @@
> +
> +/* ..... junkStatus column ..... */
> +
> +.junkStatusHeader {
> +  -moz-padding-end: 3px;

Should we really everything which is in all themes the same move to a shared file. I'm asking this because all themes have additional properties to this class and we can't remove the whole rule. #tabmail:-moz-lwtheme for example will be deleted on two themes and OSX has additional properties. This makes sense for me to share this rule.

You can leave it now if you want, but I think in the future we shouldn't share such things.
Attachment #8351294 - Flags: review?(richard.marti) → review+
Addressed review feedback.
Attachment #8351294 - Attachment is obsolete: true
Attachment #8351384 - Flags: review+
(In reply to Richard Marti (:Paenglab) from comment #2)
> Comment on attachment 8351294 [details] [diff] [review]
> Move code to shared/
> 
> Review of attachment 8351294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You forgot to write "Don't open before Christmas" ;)
> 
> r+ with the comments from Magnus and me addressed.
> 
> One question I had by creating my bug 952953: Do we need the mail directory
> in shared? Couldn't we place the files directly in shared? Naturally shared
> files from addrbook, compose etc. will be placed in their subdirectories.
> Now in the early state of shared files we could adapt our patches and only
> for messenger.inc.css a tiny patch would be needed.

I've personally prefer the /mail because it keeps the URLs for images/files pretty consistent. That is, every file can be found in platform/mail/whatever. Where platform is shared/linux/osx/windows.

> 
> ::: mail/themes/osx/mail/mailWindow1.css
> @@ +38,3 @@
> >    color: -moz-DialogText;
> >    /* Draw the top border of the tabstrip when core doesn't do it for us: */
> >    border-top: 1px solid hsla(0, 0%, 0%, 0.3);
> 
> This two lines are coming from my patch from bug 952953. Your patch would
> fail applying on a tree without my patch.

Fixed.

> 
> @@ -609,5 @@
> > -#quotaMeter {
> > -  -moz-appearance: none;
> > -  min-width: 4em;
> > -  max-width: 4em;
> > -  border: 1px solid #8B8B8B;
> 
> On shared file the color is ThreeDShadow. Does this look good on OS X?

Yep, there is actually almost no difference.

> 
> ::: mail/themes/shared/mail/mailWindow1.inc.css
> @@ +54,5 @@
> > +
> > +/* ..... junkStatus column ..... */
> > +
> > +.junkStatusHeader {
> > +  -moz-padding-end: 3px;
> 
> Should we really everything which is in all themes the same move to a shared
> file. I'm asking this because all themes have additional properties to this
> class and we can't remove the whole rule. #tabmail:-moz-lwtheme for example
> will be deleted on two themes and OSX has additional properties. This makes
> sense for me to share this rule.
> 
> You can leave it now if you want, but I think in the future we shouldn't
> share such things.

Noted, I didn't change anything in this patch, but I think you're probably right. I won't do that in the future.
https://hg.mozilla.org/comm-central/rev/336db05c6bd8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.