Clean up CSS coding style

With as much venomous disdain as I have for the idea of alphabetizing CSS properties it seems that the community is moving that way and this patch does clean up quite a lot. Also, it does not make any non-formatting changes so I close my eyes, sigh loudly and mark RTBC

 /* --------------- Main Navigation ------------ */

Please fix the comments also, almost none of them conform to the standard.

Good work and thank god someone finally is doing this.

Powered by Dreditor.

+++ themes/bartik/css/style-rtl.css	24 Aug 2010 01:38:36 -0000
@@ -120,18 +132,20 @@ ul.tips {
+ * Password Meter ¶

@@ -172,24 +192,26 @@ a.button {
+ * System Tabs ¶

@@ -207,7 +229,9 @@ ul.action-links li a {
+ * Form Elements  ¶

+++ themes/bartik/css/style.css	24 Aug 2010 01:38:36 -0000
@@ -739,58 +767,60 @@ h1#page-title,
+  border-color: rgba(255, 255, 255, 0.15);  ¶
...
+ * System Tabs ¶

@@ -832,27 +862,29 @@ h1#page-title,
+ * Messages ¶

@@ -867,19 +899,25 @@ div.error, tr.error {
+ * Breadcrumbs  ¶
...
+ * User Profile  ¶
...
+ * Password Meter ¶

@@ -901,7 +939,9 @@ div.password-confirm {
+ * Buttons   ¶

@@ -928,15 +968,17 @@ a.button:visited,
+ * Form Elements  ¶

whitespace issues ... other than that this is, sigh, RTBC

Powered by Dreditor.

One more coding-style issue to fix if the CSS coding standards are to be followed:

Multiple selectors should each be on a single line, with no space after each comma.

From style.css:889 (post-patch)

div.status, tr.status {
  background-color: #c7ffc0;
  border: 1px solid #89d47f;
}
div.warning, tr.warning {
  background-color: #fcfca7;
  border: 1px solid #e1c46b;
}
div.error, tr.error {
  background-color: #ffcccc;
  border: 1px solid #fb6b6b;
}

And a consistency nitpick:

style.css:393

  background: rgba(255, 255, 255, 1);

style.css:400

  background: rgba(240, 240, 240, 1.0);

Should it be 1 or 1.0?

Another nitpick: in style-rtl.css:12

blockquote{

Should be

blockquote {

Thanks for the update, I took another look at the stylesheets and found a real issue. I searched for non-comment lines that do not end with braces, commas or semi-colons:

$> grep -nE '^[^*]*[^,{};*/]$' themes/bartik/css/*.css
themes/bartik/css/style.css:728:  margin-left: 0
themes/bartik/css/style.css:788:  background: rgba(0, 0, 0, 0.15)

These lines are sneaky as their meaning depends on where in block they are located. And if you take a closer look at the patch it actually reorders the margin-left: 0 line:

@@ -697,8 +725,8 @@ h1#page-title,
 }
 #footer-columns .content ul {
   list-style: none;
-  padding-left: 0; /* LTR */
   margin-left: 0
+  padding-left: 0; /* LTR */
 }
 #footer-columns .content li {
   list-style: none;

That hunk makes the CSS invalid. :( I think both lines found above should be appended with semi-colons to prevent such mistakes.

This is good work, but why don't we hold off for a bit here, there's some big knarly patches sitting in Bartiks queue at the moment and if this gets committed we'll need to re-roll those patches (a lot of work in some instances) and about 20 other smaller ones. I know this needs to be done but the timing is not real good at the moment, especially since we seem to have a human resource issue with patch reviewers.

Yep its that time again, I'm pretty much slammed the next 5 - 6 days (I'm moving to another country...).

I found one erroneous alphabetization. Height should come after float.

 .tabs ul.primary li a {
-  color: #000;
   background-color: #ededed;
-  height: 1.8em;
-  line-height: 1.9;
+  color: #000;
   display: block;
+  height: 1.8em;
   font-size: 0.929em;
   float: left; /* not LTR */
-  padding: 0 10px 3px;
+  line-height: 1.9;
   margin: 0;
+  padding: 0 10px 3px;
   text-shadow: 0 1px 0 #fff;
   -khtml-border-radius-topleft: 6px;
-  -moz-border-radius-topleft: 6px;
-  -webkit-border-top-left-radius: 6px;
-  border-top-left-radius: 6px;
   -khtml-border-radius-topright: 6px;
+  -moz-border-radius-topleft: 6px;
   -moz-border-radius-topright: 6px;
+  -webkit-border-top-left-radius: 6px;
   -webkit-border-top-right-radius: 6px;
+  border-top-left-radius: 6px;
   border-top-right-radius: 6px;
 }

 .region-header .block-menu li a:hover,
 .region-header .block-menu li a:focus,
 .region-header .block-menu li a:active {
-  text-decoration: none;
   background: rgba(255, 255, 255, 0.15)
+  text-decoration: none;
 }

 .region-header #block-user-login .form-actions {
+  clear: both
   margin: 4px 0 0;
   padding: 0;
-  clear: both
 }

 #footer-columns .content ul {
   list-style: none;
-  padding-left: 0; /* LTR */
   margin-left: 0
+  padding-left: 0; /* LTR */
 }

eek! :(

See comment #9. To prevent this from happening again, we should probably fix themes/bartik/css/style.css:892: background-color: rgba(0, 0, 0, 0.15), i.e. add a trailing ";".

Trailing semi-colons are mandatory for this reason, so lets fix all those as part of the cleanup, good spotting!

Patch looks great.

Yes, looking good :)

Here are some more style nitpicks that could be included to make the patch even better:
(line-numbers after applying patch drupal-889256-20.patch)

style-rtl.css:11:

blockquote{

Missing space after selector.

print.css:38:

#triptych-first, #triptych-middle, #triptych-last {

Multiple selectors should be on separate lines.

maintenance-page.css:52:

.maintenance-page  h1#page-title {

Double space in selector.

style.css:735:

  text-shadow: 0px 1px 0 #fff;

and style.css:1413:

  padding: 0px 5px 5px;

Redundant px (0px → 0).

style.css:485:

  background: rgba(255, 255, 255, 1);

style.css:493

  background: rgba(240, 240, 240, 1.0);

Inconsistency (either 1.0 or 1):

colors.css:33:

  color: #0071B3;

Downcase hex triplet.

style.css:1385:

 color: #000000;

Wrong indentation and could be shortened to #000.

Some 0.x values are shortened to .x, but most aren't, should probably be normalized. See:

$> grep -nE '\s\.[0-9]' themes/bartik/css/*.css

style-rtl.css:196:

  zoom: 1;

IE-only statements here, OK?

Some more color hex triplets can be shortened.

You alphabetized the selectors and reformatted the comments to PHP format (instead of the industry standard for CSS)?

/me drops to the floor in defeat.

I succeeded for 11 months in not doing those two things. (Yes, I was told to many, many, many times.)

I *hate* the fact that in our community a few PHP coders have dictated how CSS is "supposed to be formatted" against the wishes of many front-end development professionals. It really, really bugs me. Just another example of how designers and front-end people are second class in the Drupal world. CSS Pros (from outside the Drupal world) who are looking at Drupal to evaluate whether or not they want to use it will look at this CSS and say "why in the world is it like this?"

:(

We have coding standards for core, its really as simple as that and no one project can just ignore that or we'd have anarchy - the comments were OK but not inline with every other CSS file in Drupal core. As much as I dislike CSSdoc as our semi adopted bastardized standard we haven't really come up with anything better, we should try harder, but for now its what we got.

I should clarify, when I said "a few PHP coders" I didn't mean you Tim. Or anyone working on this issue. Or on Bartik, actually. I meant the people who "won" their ideas for this: http://drupal.org/node/302199 over protests to format comments differently or not alphabetize the properties — in this discussion: http://groups.drupal.org/node/14421 (an example: John Albin's comment against alphabetizing the properties that 'lost': http://groups.drupal.org/node/14421#comment-49849)

Also, because there was disagreement, that "standard" is just a draft. It is not an approved standard. And yeah, I know that discussion ended in stalemate. :( And I don't have any good answer right now.

Yeah, and my complaints are not intended to prevent this patch from going in. I'm just complaining. Perhaps we can get a better agreement on CSS coding standards worked out before D8. Meanwhile, getting D7 out the door is the priority, and code formatting debates should not hold anything up.

So, this doesn't get a +1 from me, but it also doesn't get a -1. I just give it a 0. And expect to see it committed. For sure, some of what this patch does is really needed. Code cleanup is good.

I do want to thank everyone who's been working so hard on Bartik. You get ++ all around! :D

FWIW I'm neither for or against alphabetized properties, my CSS probably comes like that a lot since I tend to be a heavy user of Firebug, but when I write freehand directly into CSS files its never like that... in the past I have supported it, however now I am less supportive, I think the original argument was for scan-ablilty, but I think if you're writing CSS at this level (core) it doesn't really matter, you should be able to grok the CSS in a flash no matter what order it comes in.

Just for later, line 167 in style.css seems to have a superflous '}' . And in line 1301 padding-bottom: none; doesn't validate and is dropped by firefox, at least.

This is probably now a good time to do a code review / cleanup.

I didn't check to see if anything was missed ... but everything covered in #38 is squeaky-clean and good to go

yummy +1

Committed to CVS HEAD. Thanks.