Posted by webchick on September 2, 2007 at 10:46pm
10 followers
| Project: | Drupal core |
| Version: | 5.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Tresler |
| Status: | closed (fixed) |
Issue Summary
Color module is missing a help page at admin/help/color.
We need a fully-blown help page here, like admin/help/aggregator: one sentence description, paragraph of more detailed description.
Comments
#1
Hah! I love the low hanging fruit of the critical drupal queue - I'm on it!
#2
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.
#3
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 vs. (note capitalisation), and there are lower-cased instances of (IMHO/AFAIK, Drupal should be capitalised).
#4
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.
#5
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.
#6
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 ().
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.
#7
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.
#8
The patch applies cleanly and works as advertised, with the following text:
Since people haven't spoken up about anything needing to be rephrased, I'm saying it's good as it is - except for a which should be in the second paragraph. This is changed in the attached patch, as the only change from the patch in #7. Marking RTBC.
#9
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.
#10
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.
#11
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.
#12
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.
#13
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?
#14
Revised help text.
#15
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 in blurb 2, which I fixed in #8.
I'm also not very fond of "resave" in – how about ?
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.)
#16
Fixed the double quotes.
Fixed the 'theme's' and resave issues.
Handbook page placeholder created, aliased, and patch updated to point to it.
#17
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. :)
#18
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.
#19
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!).
#20
Thanks, committed.
#21
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
$sectionstuff, the only difference between this and the D6 patch is a small correction for code style which coder caught: The D6 patch saidt(...). '</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. :)#22
Oh, committed a coding style fix to Drupal 6 with the concatenations fixed at the end of both lines of the help text.
#23
DrupalTestbedBot tested Freso's patch (http://drupal.org/files/issues/color_help_D5.patch), the patch passed. For more information visit http://testing.drupal.org/node/122
#24
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).
#25
Committed to 5.x.
#26