CSS coding standards: Recommend lowercase not UPPERCASE hex colors

I also prefer lowercase, personally. I find it a lot easier to distinguish letters from numbers.

+1 for lowercase hex colors

+1 for lowercase, too.

- Lowercase is easier to read for humans. EVERY TEXT IN UPPERCASE TAKES ADDITIONAL BRAINPOWER TO READ.

- The only app that ever produced uppercase hex colors is PhotoShop. Sometimes I make sure to convert them before saving, sometimes not.

In the end, I don't really care. It should be a recommendation, not a rule.

Theres no technical reason to keep it in upper or lower case its actually perfectly valid to even combine these to

Except that color module is case sensitive when parsing your stylesheet and replacing/shifting colors.

+1 for for lowercase

I also prefer lowercase

Resetting status. I have always used uppercase, but I'm not particularly attached to either standard. Doesn't make much difference to me as long as we're consistent in all CSS files in core.

Looks like we have an agreement here. Slightly improving the issue title, and adding the more common coding standards tag.

Except that color module is case sensitive

Does that mean there are code changes required for this coding standards change?

Does that mean there are code changes required for this coding standards change?

no it means that it will guarantee color module(s) will always work because it's always lowercase :)

Let's call this RTBC then.

I agree with lower-case. There is no patch though. Want me to virtually commit this? :)

We need to change http://drupal.org/node/302199 but I dan't edit it. Stupid permissions...

Haha, thanks @Dries :)

I can't edit the page either. @sun do you have permission?

Updated accordingly: http://drupal.org/node/302199

Thanks!

Was a bit faster than the King ;-)

Here is a patch that changes all uppercase color codes to lowercase.
While looking through the files I noticed that the rule "Multiple properties should be listed in alphabetical order." does not apply to all css-files in core. Do we want to fix this, too?

---
edit: ups, beltofte was faster

Looks like the patch in #17 is better than mine. I missed some colors in comment_hacks.css.optimized.css.

Another issue I discovered is that both bartik and seven some places are not using shorthand hex codes. It would make sense to fix this too.

Updated my patch with the missing colors in comment_hacks.css.optimized.css + fixing two hex codes in Seven to use shorthand style.

Hey guys... I'm sorry, but can we wait on this? It would cause some other patches in progress to need to be re-rolled and that would really suck. The plan was not to provide a patch here (that's why it was marked fixed after the docs change was done), but to outline the standard.

No problem :-) We can fix it later.

Yay, thanks so much for understanding! :D

Just talked with Jacine in IRC. I'm hesitant to just postpone this, lest it fall through the cracks and we never come back to it. So let's postpone this with the stipulation that it will be reopened on Jan 15. That will give the other patches time to land so we don't create conflicts.

Whoops.

Reopening per #23.

#19: hex_colors_lower_case_19.patch queued for re-testing.

#19: hex_colors_lower_case_19.patch queued for re-testing.

clearly this standard hasn't been enforced on new commits, I will do a quick clean-up here

re-rolled, caught a lot more stuff since many new commits have been made.

hmm, I'm having trouble getting the testing module running in my local setup.. until I can get this working, I won't be able to find out the issue.

Looks like the only one that's failing is a color module test. ColorTestCase->_testColor(), to be exact, and the problem stems from the changes made to bartik.

--- a/core/themes/bartik/css/style.css
+++ b/core/themes/bartik/css/style.cssundefined
@@ -42,7 +42,7 @@ del {
   text-decoration: line-through;
 }
 tr.odd {
-  background-color: #dddddd;
+  background-color: #ddd;
 }
 img {

Here's an example of the problem. Do not shorten hex color codes from 6 digits to 3 digits, even if it's technically allowed. The ONLY thing that we're doing with this patch is making the letters lowercase.

Thanks Kris :)

I don't think we should touch jQuery UI's code either, since that is an external library. Anyone else have thoughts on that?

Oh yeah, good catch. No changes to jquery javascript or css files please. :)

oops! sorry about that, will re-roll

excluded the jquery ui css, should have caught that first time around.

sorry, ignore this patch :\ forgot to remove the shortened hex codes.. thought this was ok, since it was mentioned being done in #19

ok, no shorthand changes, no external lib changes

Go testbot, go!

Code looks good, tests pass, RTBC.

Whoops, changed a color here:

 tr.taxonomy-term-divider-bottom {
-  border-top: 1px dotted #CCC;
+  border-top: 1px dotted #eee;
 }

good thing you guys are here to keep me in line

Looks good.

This patch will go out of date really quickly considering the amount of different core modules affected.

A number of the major bugs are RTBC or to be backported, so hopefully we should below thresholds soon. (Unfortunately the fact that this patch is so prone to needing a reroll is the same reason we might need to wait to commit it until we're back below issue thresholds.) Thanks for staying on top of it!

Actually, now that we have a dedicated committer for these kinds of patches, I think they're fine to go in even when we're over thresholds. Just eyeball the RTBC queue first to make sure it's not likely to conflict with anything else in there.

Assigning to Jennifer to take a look at.

Er. This isn't a docs patch? I mean, not that it makes any difference really.

http://buytaert.net/jennifer-hodgdon

"Her responsibility will be solely around documentation and code style patches" :)

OK. :)

#44: core-hex_colors_lower_case-1360790-44.patch queued for re-testing.

The patch looks fine to me too. I'll look at the RTBC queue and assuming nothing else seems to conflict, get it in hopefully today.

There are currently at least 5 issues in the RTBC queue that include CSS files that are also in this patch. They *might* not conflict, but I didn't feel like trying them all... So I guess I'll just wait on this one.

k, I'll watch for updates in case a quick re-roll is needed.

We're under thresholds, so I committed this.