Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
FileSize
4.06 KB

Splitting from #2062439: Provide listing of custom block entities to keep the changes there in scope

Status: Needs review » Needs work

The last submitted patch, custom-block-help.1.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

#1: custom-block-help.1.patch queued for re-testing.

jhodgdon’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

Entirely missing hook_help() is a critical issue (jumped the docs gate) as per catch.

A few comments on the patch:

a) It seems like the hook_help() for the Block module should mention the custom blocks module, rather than just taking the section out about creating custom blocks.

b) The Custom Block module help's About section can call them "blocks" not "boxes of content" and reference the Block module help for more information about blocks in general.

c) The Uses section seems to contradict itself. First it says you administer these things on the Custom Blocks page, and then later on it says they're on the Blocks page. ??? Either it's wrong or it's confusing, or both.

larowlan’s picture

not as confusing as the UI apparently #2062715: [META] Many UI/UX issues with custom blocks.

alexpott’s picture

tim-e’s picture

Status: Needs work » Needs review
FileSize
4.88 KB

Here is another pass at this taking into consideration the comments from #4.

tim-e’s picture

Sorry about that, had the wrong help URL, fixed.

Status: Needs review » Needs work

The last submitted patch, 2062761-add-hook-help-custom-block.patch, failed testing.

tim-e’s picture

Status: Needs work » Needs review
FileSize
4.2 KB

Ok here is another pass at this.

pameeela’s picture

Status: Needs review » Needs work

@tim-e, this looks good!

Just two comments.

+++ b/core/modules/block/block.moduleundefined
@@ -61,7 +61,7 @@ function block_help($path, $arg) {
+        $output .= '<dd>' . t('Custom blocks allows you to create different block types, with their own fields and display settings. See <a href="@custom-blocks">Custom blocks</a>.', array('@custom-blocks' => url('admin/help/custom_block')));

I'd change this to:

'Users with the Administer blocks permission can add custom blocks, which are then listed on the Blocks administration page. Once created, custom blocks behave just like default and module-generated blocks. You can also create custom block types, with their own fields and display settings. See Custom blocks.'

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -14,14 +14,21 @@
+      $output = '<p>This page provides a list of all custom-block content on your site. From here you can add, edit or delete custom block content.</p>';

Change the second instance of 'custom block content' to just 'custom blocks'.

jhodgdon’s picture

Also, when referring to the module, say:

... the Custom Blocks module ...

And when linking to the help page for another module, the link text should be "the help page for the Foo module".

And... What's a "default" block, as opposed to a "module-generated" block? I think all blocks come from modules?

catch’s picture

Category: bug » task
lostkangaroo’s picture

Title: Add hook_help() for custom_block.module » Update hook_help() for custom_block.module
Priority: Critical » Normal

Looking into custom_block last night I noticed that a hook_help had been added with #2062439: Provide listing of custom block entities. So now this becomes an update task.

batigolix’s picture

jhodgdon’s picture

Title: Update hook_help() for custom_block.module » Update hook_help() for block and custom_block modules

ifrik decided to close #2029731: Improve help for blocks module as a duplicate, so the scope of this issue has now expanded to be both block and custom_block modules together. Make sure to review and fix both!

Alternatively, you can open up that other issue again and move the part of the patch here dealing with block.module to that issue.

ifrik’s picture

It would actually make more sense to treat the two issues as separate, and I'm happy to open #2029731: Improve help for blocks module again.
Unfortunately the patch in #10 fails for me (on line 14?), so I can't see what's already done, and/or what could be moved to a separate patch for blocks itself.

jhodgdon’s picture

OK, go ahead and open up the other issue, change the title here, and put the other issue back on the meta-issue. :)

If you open the patch file in #10 in your browser, you can see that the line that was changed was in Uses:

-        $output .= '<dd>' . t('Users with the <em>Administer blocks</em> permission can add custom blocks, which are then listed on the <a href="@blocks">Blocks administration page</a>. Once created, custom blocks behave just like default and module-generated blocks.', array('@blocks' => url('admin/structure/block'))) . '</dd>';
+        $output .= '<dd>' . t('Custom blocks allows you to create different block types, with their own fields and display settings. See <a href="@custom-blocks">Custom blocks</a>.', array('@custom-blocks' => url('admin/help/custom_block')));

This change is not grammatically correct and it fails to call the module "Custom Blocks" (note capitalization), but something like it should go into the Block module.

ifrik’s picture

Title: Update hook_help() for block and custom_block modules » Update hook_help() for custom_block modules

Title changed so that this issue only deals with custom blocks, while #2029731: Improve help for blocks module will deal with blocks.

jhodgdon’s picture

So we need to remove the Blocks module section from the patch above, and there have been quite a few review comments above.

tim-e’s picture

Assigned: Unassigned » tim-e
tim-e’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Ive removed the 'block' help from the patch and also addressed all the feedback since #10. I think I have covered everything but I suspect there might be some more work needed on the wording of the help text.

jhodgdon’s picture

Status: Needs review » Needs work

Hey, sorry this took me so long to review! I'm just catching up from the help sprint at Prague...

This patch seems to have gotten rid of the help for page 'admin/structure/block/custom-blocks/types' -- was that intentional?

Other than that, it looks pretty good! A few small things:

a) The parens on the sentence about Blocks in the About can probably be removed.

b) I don't think the Block module tells about *creating* blocks. Isn't that more about managing blocks, and this module (custom blocks) or other modules do the creating?

c) There is no link to the on-line docs page for this module. See guidelines at
https://drupal.org/node/632280

d) The first Uses topic refers to some "administration pages" but I think it is just one page?

e) I don't think we need to say the "Once created... behave just like module-generated blocks" sentence twice?

Thanks!

ifrik’s picture

Thanks, there is so much more in custom blocks then I expected :)

1) Picking up on the comments in #10:
a) and b): @jhodgdon 's comment is correct. The block module is only about placing and mananging blocks. See #2029731: Improve help for blocks module for the proposed help text.
How about rewriting the about section without the brackets to something like this?
The Custom Block module allows you to create blocks of content. Once created, they behave just like module-generated blocks. For more information on placing and managing blocks see the <link>Blocks module help page</link>.

2) I'm not completely sure about the wording "module-generated". Would we call a block that's generated by Views be "module generated"? Alternatively you could say something like "automatically generated". The main point we need to bring accross here is that there are blocks that are made somewhere else, or so.

2) "Listing custom blocks and types" - Maybe as first item under Uses, we should describe the Custom block library as the place that lists all custom blocks and types, and from where you can add blocks and types. That's also because the pages to add blocks and types, don't actually show up in the admin menu,.

3) "Creating blocks": Instead of saying that "Once created, they behave like module generated blocks", I would be more specific. Maybe:
Once created, custom blocks are listed on the <link>Block Layout page</link> where they can be placed and managed like [other/module/automatically generated] blocks.

4) "Creating custom block types". Maybe split that into two sentences. First: the types can be created on the Custom block types page. It doesn't need the "administration" in there.
And second: saying that once you have created a custom block, you can add and manage fields and change its display settings in the Types listing of the Custom Block Library. With a link to the Field and Field UI pages. (See the Link module for standard wording.

5) I've got a second button to add a block type in the types list. Any idea whether that is a bug, or whether that button is supposed to do something different?

larowlan’s picture

I think its no-longer clear from #24 and #23 what the remaining tasks are here, they seem to conflict.
Can we get an issue summary update of the remaining tasks.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

OK... I decided it was easier just to make a new patch than to make a list of what needed to be done.

So this is a clean patch that only changes the main module help (previous patches had other changes, which are not desirable), and I think it addresses all the concerns above. I do not think an interdiff is useful at this point, since every line in this patch is different from previous patches.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

I also updated the issue summary, which was quite innacurate at this point. It is now just a clean summary saying we need to update the help according to the guidelines.

batigolix’s picture

Links are okay & text is conform the guidelines.

batigolix’s picture

[removed duplicate comment]

larowlan’s picture

Assigned: tim-e » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Like the cross-linking to avoid repeating ourselves.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Issue summary: View changes

Update summary, which was out of date

Status: Fixed » Closed (fixed)

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