Help File Fixup: Block module

Applied patch and it looks good. Few small suggestions:

+++ modules/block/block.module	14 Nov 2009 07:19:46 -0000
@@ -17,17 +17,16 @@ define('BLOCK_REGION_NONE', -1);
+      $output .= '<dd>' . t('When working with blocks, remember that all themes do <em>not</em> implement the same regions, or display regions in the same way. Blocks are positioned on a per-theme basis. Disabled blocks, which have not been placed in a region, are never shown.') . '</dd>';

To clarify for newbies, maybe add 'not visible to anyone but administrators' or 'not visible to the public'? or is that overkill?

+++ modules/block/block.module	14 Nov 2009 07:19:46 -0000
@@ -17,17 +17,16 @@ define('BLOCK_REGION_NONE', -1);
+      $output .= '<dt>' . t('Controlling visibility') . '</dt>';
+      $output .= '<dd>' . t('Blocks can be configured to be visible only on certain pages, only to users of certain roles, or only on pages displaying certain <a href="@content-type">content types</a>. When allowed by an administrator, specific blocks may be enabled or disabled on a per-user basis using the <em>My account</em> page. Some dynamic blocks, such as those generated by modules, will be displayed only on certain pages.', array('@content-type' => url('admin/structure/types'))) . '</dd>';

Or "Administrators can also allow specific blocks to be enabled or disabled by users when they visit the My account page."

But otherwise patch looks great :)

re: my first comment, I was (badly) suggesting that although that the blocks aren't shown publicly, to the admin person reading the help, they can see the disabled block entry on the blocks administration page; it's just that it's listed down in the Disabled section. If there's an elegant and concise way to explain that, cool. :)

Looks good. RTBC (since it was already given the thumbs up by @jhodgdon)

This looks really great! But one thing I'd add is a section in the "Uses" area about the fact that you can also add your own custom blocks.

Looks great to me...

(drumroll...)

COMMITTED TO HEAD!

Great job, ariane! What an AWESOME first core patch. :D

Oh, shoot! Sorry about that. :)

GREENED!

don't want to spoil the party but Jennifer repeatedly commented that for uniformity's sake all help texts should start with "The xyz module blah blah blah". see e.g. comment #4 in #633808: Help File Fixup: Image module

your version starts with "Blocks are boxes of content rendered into an area, or region, of one or more pages of a website. "

as this is a complex sentence already i can't suggest an improvement without breaking too much. a conservative attempt to rewrite would be:

"The block module allows you to put boxes of content ('blocks') into an area, or region, of one or more pages of a website."

and line 21 misses a dot

$output = '<h3>' . t('About') . '</h3>';

=>

$output .= '<h3>' . t('About') . '</h3>';

rtbc

Cool, committed to HEAD, with the exception that I added the . before the = on the h3 line, per batigolix. While it's true it's unnecessary since it's the first line of the help text, the rest do this pattern:

$output = '';
$output .= 'First line blah blah...';

So this makes it consistent.

Just a note that when something's RTBC, you can go ahead and mark it that way, and the bot will still test it. If it fails anything, it will happily mark it back to needs work for you. ;)

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 or we need to change the standard.

The UX folks approved the standard, way back when. Possibly on purpose, and possibly because they have a standard that we don't know about... We need to confer with them.

And let's continue discussion back on http://drupal.org/node/537828#comment-2301622
#537828: Help text for core modules - update to conform to new standard

I'm reopening this issue, as the Block help screen has two problems:

a) Refers to "The default theme Seven"... Seven is not really the default theme for Drupal. It is set up as the default administration theme if you use the non-expert/default install profile, but if you use the expert install profile, it isn't set up as the default administration theme (at least not currently, though maybe that is a bug in the expert profile?). Anyway, in any case, it isn't the "default theme" for Drupal, that would be Garland. So the help screen should probably say "The administration theme Seven".

b) The top section says "a block may appear in any one of these areas" -- should be consistent and use the official term "Region" here.

Just a note that in another help screen (Shortcut), we refer to Seven as "core Seven administration theme".

Here's a patch that changes the About section to read:

The Block module allows you to create boxes of content, which are rendered into an area, or region, of one or more pages of a website. The core Seven administration theme, for example, implements the regions "Content", "Help", "Dashboard main", and "Dashboard sidebar", and a block may appear in any one of these regions. The blocks administration page provides a drag-and-drop interface for assigning a block to a region, and for controlling the order of blocks within regions. For more information, see the online handbook entry for Block module.

That's quite a different patch from the one I submitted. Does it need a review perhaps?

Oh, I see, you just capitalized Blocks administration page in a couple of places that I missed in my review. Agreed then, RTBC.

There's no way that particular patch caused that particular test to fail, so I requested a retest.

Committed to CVS HEAD. Thanks!