459668 - Clean up formatting.css

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect)

Description

[[e-mail]]

Portions of formatting.css are obsolete and the rest can be moved to global.css as per bug 459457.

Comment 1

[Tyler Downer [He/Him]]

Attachment: patch v1

I think this does most of what was wanted. Not sure if their were any other things that needed to be kept (or removed).

Comment 2

[Tyler Downer [He/Him]]

Attachment: Correct patch v1

Sorry, forgot to do remove to get the formatting.css out of here. Same stuff applies.

Comment 3

[[e-mail]]

Comment on attachment 354423 [details] [diff] [review]
Correct patch v1

Thanks for having a go at fixing this bug.

Possibly because you simply cut and pasted from formatting.css into global.css, my git-apply complained that many "new" lines contained whitespace errors (typically trailing whitespace), it would be neat to avoid that.

>diff --git a/suite/themes/modern/global/formatting.css b/suite/themes/modern/global/formatting.css
>deleted file mode 100644
So, as I see it, too many rules from this file fell by the wayside.

>-separator, separator[orient="horizontal"] {
I'm not sure about the separator rules. I wasn't really sure what was going on.

>-.small-margin {

>-.plain {

>-description, label {

>-description {

>-label {

>-description[disabled="true"],
>-label[disabled="true"] {
All of these rules seem to be missing.

>-.header {
This one, strangely, seems to have been duplicated.

>-.monospace {

>-.indent {

>-.box-padded {

>-.spaced {

>-.wizard-box {
Again, these ones seem to have gone missing.

>-.text-link {

>-.text-link:focus {

>-.text-link:hover {

>-.text-link:hover:active {

>-.text-link[visited="true"] {
And these ones also seem to have been duplicated.

Comment 4

[Tyler Downer [He/Him]]

Thanks, I will copy it better. Some of those were somewhat foggy on whither they needed to be kept (i.e., not being used right now, but listed as available, etc.). i will have another go in the next few days.

Comment 5

[Tyler Downer [He/Him]]

Attachment: Patch v2

OK, version 2. This cleans up the whitespace (yes I was lazy and just copied and pasted it into global). Removes the dupes (again, lazy), and puts in the missing stuff from comment #3. I left the separator stuff in for now, as it is not clear what the issue is with that. A new bug can always be filed to remove it later when it becomes clearer. 

One thing I am not sure about is this:
>/* deprecated */  
>window.dialog { 
You will see I put a note in the patch asking if this as needed anymore. It may just be easiest, while I'm in global, to remove it if we don't need to keep it. It could be listed somewhere as available though, that is my only concern (or used in an obscure file somewhere).

But I think this address most of the issues above, anything else?

Comment 6

[[e-mail]]

(In reply to comment #5)
>One thing I am not sure about is this:
>>/* deprecated */  
>>window.dialog { 
>You will see I put a note in the patch asking if this as needed anymore.
Sadly it is, e.g. http://mxr.mozilla.org/comm-central/source/mailnews/base/search/resources/content/CustomHeaders.xul#43

Comment 7

[Tyler Downer [He/Him]]

Comment on attachment 355044 [details] [diff] [review]
Patch v2

OK, I will take out my comment.

Comment 8

[Tyler Downer [He/Him]]

Attachment: Patch v2.1

This should do it

Comment 9

[[e-mail]]

Comment on attachment 355135 [details] [diff] [review]
Patch v2.1

This is almost correct, but I spotted a few little fixes:

>+
>+/*::::::From formatting.css::::::*/
>+
>+/*:::::Inset Areas::::::*/
>+
These aren't in the same style as the rest of the file.

>+/* ..... standard separators ..... */
>+separator, separator[orient="horizontal"] {

>+/* ..... thinner separators ..... */
>+separator.thin, separator.thin[orient="horizontal"] {

>+/* ..... groove separators ..... */
>+separator.groove,
Please put blank lines in after these comments.

>+.icon-dropmarker {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+}
We don't need this one, you can remove it.

Comment 10

[Tyler Downer [He/Him]]

Attachment: Patch v2.2

I think this does it.

Comment 11

[Tyler Downer [He/Him]]

(In reply to comment #9)
>>+
>>+/*::::::From formatting.css::::::*/
>>+
>>+/*:::::Inset Areas::::::*/
>>+
>These aren't in the same style as the rest of the file.

I left them that way because if was a different section (You notice I put 6 :s in the formatting comment, unlike the 5 in the rest).

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> I left them that way because if was a different section (You notice I put 6 :s
> in the formatting comment, unlike the 5 in the rest).

Neil was referring to the missing spaces around the colons and capitalization (compare "Inset Areas" with "formatting" and "separator rules" below). Furthermore the "Inset Areas" line sports 5 colons before and _6_ after the title.

As for the "From formatting.css" heading, what's that one needed for in the first place? This isn't really relevant information beyond this bug, is it?

Comment 13

[Tyler Downer [He/Him]]

the formatting heading is for those who may be lost in finding the old css styles. It could be removed. I can clean up the comment.

Comment 14

[[e-mail]]

Comment on attachment 355183 [details] [diff] [review]
Patch v2.2

>+/*::::::From formatting.css::::::*/
If you want to keep this, then I think = signs would work best i.e.
/* ===== formatting.css ===== */
(maybe saying from or previously or something, I'm not really sure.)

>+/*:::::Inset::::::*/
And this needs to be the original header i.e.
/* ::::: inset areas ::::: */

>+/* reduced margin for some UI */
>+
>+.small-margin {
And an unwanted blank line crept in here (no ..... or ::::: so no blank line).

r=me with those fixed.

Comment 15

[Tyler Downer [He/Him]]

OK, i will fix those this afternoon.

Comment 16

[Tyler Downer [He/Him]]

Attachment: Patch for checking

This does the comment cleanup. Ready for checkin.

Comment 17

[[e-mail]]

Comment on attachment 355246 [details] [diff] [review]
Patch for checking

>+/*====taken from formatting.css====*/
Sorry to be so nitpicky, but these should be 5 = signs and spaces i.e.
/* ===== taken from formatting.css ===== */

>+/*::::: inset areas :::::*/
And there are two missing spaces i.e.
/* ::::: inset areas ::::: */

Comment 18

[Tyler Downer [He/Him]]

Attachment: Patch for check in 9really) [Checkin: Comment 19]

Ok, suit yourself. NP, here you go.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 355247 [details] [diff] [review]
Patch for check in 9really)
[Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/f89d29d8fa06