Closed Bug 459668 Opened 16 years ago Closed 15 years ago

Clean up formatting.css

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.0a3

People

(Reporter: neil, Assigned: tdowner)

Details

Attachments

(1 file, 6 obsolete files)

Portions of formatting.css are obsolete and the rest can be moved to global.css as per bug 459457.
Keywords: helpwanted
Whiteboard: [good first bug]
Attached patch patch v1 (obsolete) — Splinter Review
I think this does most of what was wanted. Not sure if their were any other things that needed to be kept (or removed).
Assignee: nobody → tyler
Status: NEW → ASSIGNED
Attachment #354412 - Flags: review?(neil)
Attached patch Correct patch v1 (obsolete) — Splinter Review
Sorry, forgot to do remove to get the formatting.css out of here. Same stuff applies.
Attachment #354412 - Attachment is obsolete: true
Attachment #354423 - Flags: review?(neil)
Attachment #354412 - Flags: review?(neil)
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.
Attachment #354423 - Flags: review?(neil) → review-
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.
Attachment #354423 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #355044 - Flags: review?(neil)
(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
Attachment #355044 - Attachment is obsolete: true
Attachment #355044 - Flags: review?(neil)
Comment on attachment 355044 [details] [diff] [review]
Patch v2

OK, I will take out my comment.
Attached patch Patch v2.1 (obsolete) — Splinter Review
This should do it
Attachment #355135 - Flags: review?(neil)
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.
Attachment #355135 - Flags: review?(neil) → review-
Attached patch Patch v2.2 (obsolete) — Splinter Review
I think this does it.
Attachment #355135 - Attachment is obsolete: true
Attachment #355183 - Flags: review?(neil)
(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).
(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?
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.
Attachment #355183 - Flags: review?(neil) → review+
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.
OK, i will fix those this afternoon.
Attached patch Patch for checking (obsolete) — Splinter Review
This does the comment cleanup. Ready for checkin.
Attachment #355183 - Attachment is obsolete: true
Whiteboard: [good first bug]
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 ::::: */
Ok, suit yourself. NP, here you go.
Attachment #355246 - Attachment is obsolete: true
Attachment #355247 - Flags: review+
Comment on attachment 355247 [details] [diff] [review]
Patch for check in 9really)
[Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/f89d29d8fa06
Attachment #355247 - Attachment description: Patch for check in 9really) → Patch for check in 9really) [Checkin: Comment 19]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.