Support from Acquia helps fund testing for Drupal Acquia logo

Comments

batigolix’s picture

Issue tags: +d7help

added #d7help

batigolix’s picture

FileSize
3.72 KB

new patch with missing closing tag

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Review notes:
- The other help pages all start the About section with a phrase like "The xyz module blah blah blah". This one is different; probably should conform to the other pages' style.
- For ease of translating, please put each paragraph into one t() call. It's very difficult to notice things like "end of sentence. " [the space that is part of the string to be translated) in the translation interface.
- Style/punctuation:
"By combining effects as crop, scale and desaturate effect you can create for example square, grayscale thumbnails."
should be
"By combining effects as crop, scale, and desaturate, you can create, for example, square, grayscale thumbnails."
(Doc style guidelines require the US convention of comma before "and" in a list, and "for example" always needs to be set off by commas before and after when it's used this way.)

batigolix’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

didnt see where the problem was with your 2nd remark about the paragraph not being in a t function

here is my new attempt

marcvangend’s picture

jhodgdon didn't mean that there are paragraphs outside t(), he meant that it's better to put a complete paragraph in one single t() instead of breaking it up into several t()'s. By doing that, you avoid having a space as the last character of a translatable string, which is easily overlooked by translators.

arianek’s picture

I sorta overhauled this one - got mucho consultation from @webchick on it, but it still will need a major review. Took out the naming arrays, moved that into ul (note ul/li doesn't show in the screenshot cause of the seven theme). Rewrote lots and added a section on image field. Also added the link to the handbook page, BUT it's super out of date... so not 100% sure it should be included (I just added it for consistency's sake).

arianek’s picture

Status: Needs review » Needs work

oh dear, i totally munged the indentation. will re upload.

arianek’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

fixed

arianek’s picture

FileSize
4.71 KB

dang, one more fix

batigolix’s picture

looks fine to me

i was sceptical about the whole bit about "naming convention" and the theme-list-item method to show a simple two item bullet list, but i didnt heave the guts to remove all of it.

see my remarks in #569184: Helptext Image module is not consistent with example loaded by default

arianek’s picture

yes, it seems it is from way back because quicksketch had actually written extensive documentation for his own module (rather than it being added later by someone else). So for consistency's sake, just moved that all into text to make it easier for translation, consistent, etc.

i wouldn't have done it without "permission" either, but that's what IRC is for! ;-)

so, if anyone else wants to review this go for it, otherwise batigolix, you can probably RTBC.

batigolix’s picture

Status: Needs review » Reviewed & tested by the community

set status to RTBC

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Again because of translating, I don't think it's a good idea to have line returns in the middle of text that is being run through t(). Please separate out the HTML tags as much as possible too, and just put the text inside t(), except for inline tags like EM.

batigolix’s picture

Status: Needs work » Needs review
FileSize
4.56 KB

fresh new patch

batigolix’s picture

FileSize
4.55 KB

improved patch

arianek’s picture

Status: Needs review » Reviewed & tested by the community

looks like a fine patch to me. RTBC timez.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

The standard for capitalization is not being followed. See http://drupal.org/node/632280 -- should start with "The foo module" not "The Foo module" according to standard.

batigolix’s picture

FileSize
4.56 KB

there comes another one

batigolix’s picture

Status: Needs work » Needs review

needs review

jhodgdon’s picture

Status: Needs review » Needs work

Standard for capitalization was changed, see http://drupal.org/node/537828#comment-2303764 and http://drupal.org/node/632280 -- we are now saying the name of the module should be capitalized. Sorry for the inconvenience but we need a new patch...

In the latest one, at some point it says "Image module" without "the" in front of it, which seems wrong to me. Anyway it is close..

arianek’s picture

Status: Needs work » Reviewed & tested by the community

WEBCHICK!!! pls use patch in #16 (without the caps change) http://drupal.org/files/issues/help_image_module_6_0.patch

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Just a minor tweak:

"...and to attach images to content using an image field."

"... and provides an Image field for attaching images to content."

This seemed more in-line with the earlier stuff in that sentence.

Committed #16 to HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -d7help

Automatically closed -- issue fixed for 2 weeks with no activity.