Missing help page: Color module

Hah! I love the low hanging fruit of the critical drupal queue - I'm on it!

The Color module allows the admin to change the color scheme of a site completely. However, a theme must be specifically configured to use the Color module. Garland, the default theme that drupal installs with uses it, as does its fixed-width version Minnelli. You can change the color of links, certain backgrounds, the text, and more if your theme has been built to utilize color module. Color module requires your file download method to be set to public.

It is important to note that the Color module saves a copy of the theme's style.css into the files directory, and calls it after your themes style.css. This means that if you make other style changes in your theme, you must submit your color settings again, even if they haven't changed. This has the effect of copying the theme's style to the files directory.

If you are interested in integrating the color module with your theme, take a look at the handbook page.

The words "file download method" links to the files settings admin page, and the word "handbook" links to http://drupal.org/node/108459

Input is welcome, patch attached.

AFAIK, other strings in core only have single spaces after punctuation. I seem to recall a discussion about this somewhere, but I can't remember when it was (and considering I've been more or less involved on and off since at least Drupal 4.6, I don't feel like trawling through IRC logs, mailing lists, etc. to see if I'm right), and I can't remember the outcome of it - or even if either was officially announced a standard (as official as it can be without being mentioned in the coding style document). Either way, however it ends up, it should be consistent, and it isn't currently.

Also, you're not consistent in Color module vs. color module (note capitalisation), and there are lower-cased instances of drupal (IMHO/AFAIK, Drupal should be capitalised).

Fair enough. Punctuation set to single spacing - I remember that conversation, and while I, also, don't recall a conclusion, I do remember now that HTML basically turns all double spaces into single spaces making the second space pointless.

Not sure now - what's better: Escaping single quotes in texts OR enclosing the whole into double quotes? Seems to me the latter is used around Drupal's files commonly.

I seem to recall the coding standards talking about strings, but I can't find it mentioned anywhere related to them now, so... :/
For code readability, it should be double quotes. For performance, it should be single quotes (even if the impact of this might be illegible). AFAIK, it doesn't affect the extracted translation strings either way and I believe it says somewhere that code readability is worth more than the slight performance increase in these cases.
Also, "Drupal" is still lowercased (the default theme that drupal installs with).

Anyway, the patch applies cleanly, and the help page comes up... but there's a 3rd level header titled "Color administration pages", which doesn't have any content beneath it. I don't know which pages to link to there, possibly admin/build/themes/settings and admin/settings/file-system ? Or is this generated automatically? In the latter case, it is probably a new bug and should be patched on its own and thus have no effect on this patch.

Looking at the help function in comment module it appears that the double quotes method is preferable. Changed that. Capitalized Drupal.

As for the "Administration pages" it is autgenerated - see /admin/help/help has it as well.

The patch applies cleanly and works as advertised, with the following text:

The color module allows the admin to change the color scheme of a site completely. However, a theme must be specifically configured to use the color module. Garland, the default theme that Drupal installs with, uses it, as does its fixed-width version Minnelli. You can change the color of links, certain backgrounds, the text, and more if your theme has been built to utilize color module. Color module requires your file download method to be set to public.

It is important to note that the color module saves a copy of the theme's style.css into the files directory, and calls it after your themes style.css. This means that if you make other style changes in your theme, you must submit your color settings again, even if they haven't changed. This has the effect of copying the theme's style to the files directory.

If you are interested in integrating the color module with your theme, take a look at the handbook page.

Since people haven't spoken up about anything needing to be rephrased, I'm saying it's good as it is - except for a themes which should be theme's in the second paragraph. This is changed in the attached patch, as the only change from the patch in #7. Marking RTBC.

Although I am definitely not a native English speaker, I noticed two things in this patch which should be fixed iMHO:

- A CSS file is not "called", but rather "included" or "included in the page" or "used on the page" or something.
- "The" sounds misplaced here: "integrating the color module with your theme". I'd remove "the" from there.

Again, note that I am not a native speaker, so this is rather an indication of what foreign speakers understand of the text.

Why are we linking to http://drupal.org/node/108459 ? This is a buried handbook page. If we are going to link to a handbook page then it should be added http://drupal.org/handbook/modules and properly aliased to http://drupal.org/handbook/modules/color. Otherwise it is subject to getting edited/moved and breaking the link.

sepeck:
I agree. Will someone from the documentation team do this?

Gábor:
On your first point, I agree, and the attached patch says "includes" instead of "uses". However, as a non-native English speaker meself, I do not agree with your second point. I think "the" both is and sounds perfectly fine there, and if "the" should be removed, it should be called "color.module" instead of "color module". If you insist, please tell me why "the color module" doesn't sound wrong all the other places.

My feeling about how these should be said (arbitrary sentences from the top of my head):

- "The color module allows you to set colors"
- "Drupal allows color module to inject CSS files"

I'd rather remove "the" from the first sentence then add it in the second one.

Again, this is just my "English language feeling", I am not a professional in English grammar.

I'm another non-native speaker, but let me still say an opinion on this...

To me, it definitely sounds good WITH the 'the'. I feel that a term like this deserve 'the' depending on it's position (importance?) in the sentence, so all the following sounds well to me:
"The color module allows you to set colors" - We're basically speaking about color module, not about colors...
"Drupal allows color module to inject CSS files" - We talk about Drupal in fact, not color module.
"If you are interested in integrating the color module with your theme..." - Well, this is less obvious, but still color module is the subject more than that theme IMO.

But since I heard truly scaring stories about complexity of English language rules, I think we need a native speaker here, if a really perfect wording is required. I'm not sure whether this is a blocker for this patch, though. Currently there's no help page, right?

Revised help text.

First text blurb:
As far as I can tell, it doesn't contain any apostrophes (single quotes), thus it shouldn't be in double quotes. If I didn't spot the apostrophe, <a href=\"@url\"> should probably be <a href='@url'> instead, to avoid those back slashes.

Second text blurb:
The latest patch reintroduces the themes in blurb 2, which I fixed in #8.
I'm also not very fond of "resave" in you must resave your color settings – how about you must save your color settings again?

Third text blurb:
And we still need a proper aliased handbook page for the reference in the third blurb... (I don't think this should stop the patch from going in, though I wouldn't mind keeping the issue open until a proper handbook page (with alias) has been made and a patch produced which changes the pointer to this.)

Fixed the double quotes.
Fixed the 'theme's' and resave issues.
Handbook page placeholder created, aliased, and patch updated to point to it.

Excellent! I'll try and do a proper review in the morrow – it is a rather trivial patch, so it should be possible to squeeze in the testing of it, but no guarantees. :)

The documentation looks OK. I'm not convinced we should point users to developer information (theme guidelines) though. I'd leave out the last paragraph.

The attached patch is identical to the one from #16, only with the last paragraph removed, per Dries' comment.

Tested and applies cleanly while working as advertised (just remember to clear your cache!).

Thanks, committed.

Should this be ported for D5 as well? As far as I can tell, the text applies just as well to D5 as it does to D6, so I've attached a quick port, which works fine for me on my test CVS install of D5.
Apart from the $section stuff, the only difference between this and the D6 patch is a small correction for code style which coder caught: The D6 patch said t(...). '</p>'; – notice the concatenating dot? Right. That has been moved next to the </p> string instead. I'm tempted to mark this as RTBC already, but I suppose it really does ought to have another pair of eyes upon it. :)

Oh, committed a coding style fix to Drupal 6 with the concatenations fixed at the end of both lines of the help text.

FTR: If any of you should be interested in the "administration pages" appearance behaviour I noted back in comment #6, I've (finally) filed an issue about it (#183357).

Committed to 5.x.