Support from Acquia helps fund testing for Drupal Acquia logo

Comments

batigolix’s picture

FileSize
8.18 KB

new patch with missing closing tag

Status: Needs review » Needs work

The last submitted patch failed testing.

jhodgdon’s picture

Looks fairly good, aside from the patch not applying... and I have some comments about the patch:

a)

return $output;

should be indented same as the other lines above it.

b) The other help pages begin the About section with "The xyz module ...". This one should start that way too.

c) "who it viewed, from where it was visited (referrer URL) and when it was viewed" would be clearer as:
"who viewed it, the previous page the user visited (referrer URL), and when it was viewed"
Also note that there needs to be a comma before the "and". See http://drupal.org/about/authoring

d) "The module offers four reports." before the UL, should end in : not .

e) "block that displays today\'s and all time\'s the most viewed pages," -- maybe omit "the" there? or rewrite as
"block that displays the most viewed pages today and for all time"

batigolix’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

great. thank you for the review

here's my new attempt (with all your suggestions applied)

arianek’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
132.01 KB
8.83 KB

cleaned up the language a little more and moved the managing logs info into uses - i think this is ready to go...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks great!

I found one bug, which is "To enable the statistics the option" has an extra "the" that it shouldn't. I removed it, but then realized the sentence didn't really make sense as is, so conferred with ariane in IRC and changed this bit to "To enable collection of statistics". This still is a bit awkward (probably should be in active voice rather than passive), but I think this works unless someone comes up with something better.

Committed to HEAD!

batigolix’s picture

mark that my first contribution to drupal. ever
been only using it for 5 yrs ;)

webchick’s picture

RIGHT ON!! Welcome to the team, batigolix! :D

arianek’s picture

congrats batigolix! you've been a huge help this last week, keep up the great work, we really appreciate it! :-)

jhodgdon’s picture

Status: Fixed » 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.

Needs a quick repatch unless we change the standard.

batigolix’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

here ye go

Status: Needs review » Needs work

The last submitted patch failed testing.

lisarex’s picture

Status: Needs work » Fixed

Actually consensus was reached that the caps should remain... see #537828: Help text for core modules - update to conform to new standard. So it's actually OK and commit can remain.

jhodgdon’s picture

Status: Fixed » 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... also something happened with this one in testing...

batigolix’s picture

Status: Needs work » Fixed

in that case the issue can be marked "fixed" as the current version in head *has* the correct capitalization

Status: Fixed » Closed (fixed)

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