Updated: Comment #0

Problem/Motivation

There is no listing of custom blocks outside of admin/structure/block/list/bartik/add, which is being removed by #2058321: Move the 'place block' UI into the block listing

Proposed resolution

Provide a listing of custom blocks

Remaining tasks

Write tests

User interface changes

A new listing page for custom blocks

API changes

N/A

#2058321: Move the 'place block' UI into the block listing
#2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout

Files: 
CommentFileSizeAuthor
#54 lonely-block-page-2.png30.48 KBalexpott
#52 custom-block-2062439-51.patch18.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,094 pass(es).
[ View ]
#52 interdiff.txt2.59 KBtim.plunkett
#49 custom-block-2062439-49.patch18.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]
#49 interdiff.txt2.03 KBtim.plunkett
#44 block-2062439-and-2058321.patch61.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,145 pass(es).
[ View ]
#40 Screen_Shot_2013-08-12_at_4.45.18_PM.png63.73 KBjessebeach
#40 Screen_Shot_2013-08-12_at_4.47.17_PM.png70.28 KBjessebeach
#38 custom-block-2062439-38.patch18.9 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]
#38 interdiff.txt785 bytestim.plunkett
#37 custom-block-2062439-36.patch18.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#37 interdiff.txt5.72 KBtim.plunkett
#34 custom-block-2062439-34.patch18.76 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#34 interdiff.txt3.94 KBtim.plunkett
#33 custom-block-2062439-33.patch18.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]
#33 interdiff.txt1.17 KBtim.plunkett
#30 custom-block-list-2062439-30.patch18.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,809 pass(es).
[ View ]
#30 interdiff.txt972 bytestim.plunkett
#28 custom-block-list-2062439-28.patch18.71 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,107 pass(es).
[ View ]
#28 interdiff.txt972 bytestim.plunkett
#26 custom-block-list.26.interdiff.txt1.55 KBlarowlan
#26 custom-block-list-2062439.26.patch18.58 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,948 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#24 custom-block-list-2062439.24.interdiff.txt5.1 KBlarowlan
#24 custom-block-list-2062439.24.patch18.9 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#21 custom-block-list-2062439.21.interdiff.txt11.42 KBlarowlan
#21 custom-block-list-2062439.21.patch22.4 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,801 pass(es).
[ View ]
#18 interdiff.txt2.03 KBtim.plunkett
#18 custom-blocks-2062439-17.patch18.39 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,707 pass(es).
[ View ]
#14 custom-blocks-2062439-14.patch16.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,945 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt10.35 KBtim.plunkett
#9 custom-block-list.interdiff.txt991 byteslarowlan
#9 custom-block-list.patch10.58 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,808 pass(es).
[ View ]
#2 custom-block-2062439-2.patch10.54 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,715 pass(es).
[ View ]
#2 interdiff.txt7.87 KBtim.plunkett
#1 custom-block-2062439-1.patch3.65 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

tim.plunkett’s picture

Component:block.module» custom_block.module
Issue tags:-VDC+Needs tests
StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Forget the view, custom blocks don't have integration yet. Here's the UI.

tim.plunkett’s picture

Status:Active» Needs review
Issue tags:-Needs tests
StatusFileSize
new7.87 KB
new10.54 KB
PASSED: [[SimpleTest]]: [MySQL] 57,715 pass(es).
[ View ]

Here are some tests, and other enhancements.

I think we need to get this in and go back to focusing on the Block UI as a whole.

tim.plunkett’s picture

  1. @@ -93,21 +102,6 @@ function custom_block_menu() {
    - * Implements hook_local_actions().
    - */
    -function custom_block_local_actions() {
    -  return array(
    -    array(
    -      'route_name' => 'custom_block_type_add',
    -      'title' => t('Add custom block type'),
    -      'appears_on' => array(
    -        'custom_block_type_list',
    -      ),
    -    ),
    -  );
    -}

    @@ -0,0 +1,24 @@
    +/**
    + * @LocalAction(
    + *   id = "custom_block_add",
    + *   route_name = "custom_block_add_page",
    + *   title = @Translation("Add custom block"),
    + *   appears_on = {"custom_block_list"}
    + * )
    + */
    +class CustomBlockAdd extends LocalActionBase {

    @@ -0,0 +1,24 @@
    +/**
    + * @LocalAction(
    + *   id = "custom_block_type_add",
    + *   route_name = "custom_block_type_add",
    + *   title = @Translation("Add custom block type"),
    + *   appears_on = {"custom_block_type_list"}
    + * )
    + */
    +class CustomBlockTypeAdd extends LocalActionBase {

    So I needed to add a local action, and decided to add both. Let me know if this fix for the other local action is out of scope.

  2. @@ -0,0 +1,111 @@
    +  public static $modules = array('block', 'custom_block', 'custom_block_test');

    I don't need/use custom_block_test here.

  3. @@ -0,0 +1,111 @@
    +  public function testListing() {

    Crap. I need a docblock here.

larowlan’s picture

Will review early tomorrow

benjy’s picture

Status:Needs review» Reviewed & tested by the community

I tested this and everything works as expected. Bot's happy and the sooner we get this in the better as it's blocking other critical issues.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review

I need to fix the things I mentioned at the least, and I'll let the custom_block maintainer have a go :)

benjy’s picture

@tim.plunkett ahh, I cross posted with that :)

larowlan’s picture

Have reviewed, just the items at #3 items 2 and 3 need fixing.
Lets leave #3 item 1 as is, makes sense to have them both in the same format rather than two different places.
Thanks @timplunkett for knocking this out so fast and sorry for derailing the other issue.

larowlan’s picture

StatusFileSize
new10.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,808 pass(es).
[ View ]
new991 bytes

Here's the fixes from #3
Don't credit me in the commit.

benjy’s picture

Status:Needs review» Reviewed & tested by the community

Looks good now them changes are fixed.

xjm’s picture

Status:Reviewed & tested by the community» Needs review

Sorry, but I tested this manually last night, and the workflow is crazy. @tim.plunkett and I discussed this at length and it's mostly due to outstanding issues with the custom blocks workflow, but this patch unfortunately makes it that much more confusing.

I understand and support the desire to get this in as an intermediate step to unblock #2058321: Move the 'place block' UI into the block listing, but I think we need to go about it somewhat differently.

xjm’s picture

Issue tags:+Usability, +Needs followup

Couple actionable steps off the top of my head:

  1. The redirect of the custom block creation form to the place blocks form is really unexpected. @tim.plunkett pointed out that this is out of scope for this issue, but at a minimum, it should only happen when the button is clicked from the place blocks form, and not when the form is submitted from a different path. That's the unexpected thing that completely threw me. (I use Drupal 8 every day, and I was just lost.)

    I think/hope this should just be a matter of setting a destination parameter.

    In #2058321: Move the 'place block' UI into the block listing this changes to "when the button is clicked on admin/structure/blocks".

  2. The custom block listing page can't be a child tab under custom block types. No one will ever find that.

    A start, in scope for this patch, would be to make them both top-level items. Kevin also has some workflow designs from earlier in the release cycle that we could use to inform the placement. There's an out-of-scope usability problem here as to whether custom blocks are structure or content, but it definitely can't be a child page under "custom block types".

    We have three reasonable choices:

    • They're siblings under admin/structure.
    • The custom block list is under admin/content.
    • Custom block types is the child tab.

    In this issue, we should pick one and go with it, because anything is better than what's in HEAD currently, which is nothing. Either one can reference the other in help text.

  3. We need a hook_help() on the custom block page that explains how it works.

  4. We should probably have a "Place block" operation in the list controller for each block.

  5. Let's get followups filed to expose custom block data to views (active) and to replace the list controller with a view (postponed). This patch is a slight abuse of the list controller. It works fine, even elegantly under the circumstances, but the ELC is designed for config entities, and we decided way back to standardize on Views for content entities.

  6. There are out-of-scope issues that need to be filed for the custom block workflow as it stands in HEAD. We need to be able to reference those in order to clarify the scope of this patch and show that some of the UX WTFs are existing bugs rather than regressions.

Sorry to expand the scope so much, but I really think this can't go in like it is.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

I will address 1-3 in a patch today. 4 can't be done because placing blocks is per theme, and we don't have a UI designed for picking the theme on the form.
5-6 anyone can do independently of the patch.

tim.plunkett’s picture

StatusFileSize
new10.35 KB
new16.36 KB
FAILED: [[SimpleTest]]: [MySQL] 57,945 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

This is 1 and 2, going to write a hook_help while waiting for the bot.
We can always tweak the menu structure, so I went with the least invasive and most sensical (to me) option of Custom block types is the child tab.

dawehner’s picture

@@ -0,0 +1,110 @@
+    $this->assertTitle('Edit custom block Antelope | Drupal');
...
+    $this->assertTitle('Are you sure you want to delete Albatross? | Drupal');
...
+    $this->assertText('There is no Custom Block yet.');

Don't we use t() in that case?

@@ -0,0 +1,110 @@
+
+    // Verify that the text of the label and machine name does not appear in
+    // the list (though it may appear elsewhere on the page).
+    $this->assertNoFieldByXpath('//td', 'Albatross', "No label found for deleted 'Albatross' entity.");
+
+    // Confirm that the empty text is displayed.

Shouldn't we also test that the block is really deleted and not just disappears?

tim.plunkett’s picture

15.1 Sure, doesn't matter, I guess I'll include on the next reroll.
15.2 No, this is just testing the listing. There are other tests for creating/deleting custom blocks explicitly.

Status:Needs review» Needs work

The last submitted patch, custom-blocks-2062439-14.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new18.39 KB
PASSED: [[SimpleTest]]: [MySQL] 57,707 pass(es).
[ View ]
new2.03 KB

Actually since I copied this from ConfigEntityListTest, I'm not going to add t(). We're not testing languages here, and I don't think the official language of Drupal will change soon.

Also, as far as an addition to custom_block_help(), well we don't have one yet. #1908570: [meta] Update or create hook_help() texts for D8 core modules is the existing meta for that.

xjm’s picture

We need to at least add a help text for the list controller in this patch. It is part of the docs gate, even if it's been poorly followed in a lot of cases, including in previous BAP followups.

I'll take a stab at it in a bit. Edit: or delegate it; we'll see. :)

xjm’s picture

http://drupal.org/simpletest-tutorial-drupal7#t specifies that t() should be used where @dawehner recommends. Until that's changed, we should. Not a big deal either way, but that's the standard.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockListTest.phpundefined
@@ -0,0 +1,110 @@
+    $this->assertLinkByHref('block/1');

Also, as I mentioned to @tim.plunkett last night, it annoys me that we're assuming a serial ID of 1 here. Assumptions like that about the state of the test site are bad, even if this particular one is unlikely to be broken in the foreseeable future. :) I certainly won't block the patch over it, but if it were a node I would have asked the patch author to use drupalGetNodeByTitle() or such.

larowlan’s picture

StatusFileSize
new22.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,801 pass(es).
[ View ]
new11.42 KB
@@ -73,5 +73,5 @@ function custom_block_delete_form_submit($form, &$form_state) {
+  $form_state['redirect'] = 'admin/structure/custom-blocks';

I think we should explicitly set this in the 'Edit' link in the list controller using the destination parameter. Because now the converse is weird, clicking 'add custom block' from the place block ui sends you to the wrong place. IE I think if you 'add custom block' from the block UI, you should get the 'place block' form for that block. So this changes as follows: if you click edit link in controller, destination sends you back to the listing. If you click 'add link' in list controller, you come back. If you click the add link in blocks UI, you go to the 'place block' form. To get this working I had to use hook_menu_local_tasks() until #2045267: LocalTaskBase and LocalTaskInterface has to work with routes containing parameters is in, at which point we could add and then use \Drupal\custom_block\Plugin\Menu\CustomBlockAdd::getPath(). At this point in time local action plugins don't support query parameters, which sucks, so added a @todo.

I really think this should be admin/content/blocks instead of admin/structure but thats for another discussion, as part of the meta, and is a trivial change once we get *something* in.

With respect to the views integration we had a vicious circular dependency, the custom block revision ui needed the views integration and the views integration (to be complete) needed links to the revision UI. I say we change the focus of those issues. The existing custom block views integration issue forgets about revisions, then we add views integrations for revisions and revision UI in the other issue.

Also changes 'There is no Custom Block yet' to 'There are no Custom Blocks yet' which is more correct in terms of phrasing.

And adds hook_help(), moving the salient bits from block.module (where custom_block was spawned from in D7 codebase).

And fixes #15 and #20

xjm’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.phpundefined
@@ -66,4 +66,13 @@ public function buildRow(EntityInterface $entity) {
+    // @todo Remove this once https://drupal.org/node/1981644 is in.

We should refer to the issue for which this one's marked duplicate. (Here and the other spot.)

larowlan’s picture

And move all but the list controller path from hook_help() into #2062761: Update hook_help() for custom_block modules

larowlan’s picture

StatusFileSize
new18.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new5.1 KB

Fixes #22 and #23

tim.plunkett’s picture

@@ -42,9 +42,20 @@ public function getOperations(EntityInterface $entity) {
+    // Override the default 'There is no Custom Block yet' to 'There are no
+    // Custom Blocks yet'.

This is very wrong. It will append an 's' to any translation of "Custom Block".
We just leave this well enough alone.

larowlan’s picture

StatusFileSize
new18.58 KB
FAILED: [[SimpleTest]]: [MySQL] 57,948 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.55 KB

Fixes #25

Status:Needs review» Needs work

The last submitted patch, custom-block-list-2062439.26.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new972 bytes
new18.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,107 pass(es).
[ View ]

HelpTest requires both cases for hook_help().

larowlan’s picture

@@ -8,12 +8,13 @@
+      return 'Allows the creation of custom blocks through the user interface.';

Coding standards dictate any empty line after case breaking statements (return|break)

tim.plunkett’s picture

StatusFileSize
new972 bytes
new18.71 KB
PASSED: [[SimpleTest]]: [MySQL] 57,809 pass(es).
[ View ]

Too true.

EDIT: Interdiff is wrong, I just added a blank line.

larowlan’s picture

I think we're good here, but will leave for someone else to rtbc.

benjy’s picture

When you add a custom block from admin/structure/custom-blocks and you have multiple block types you don't end up back at structure/custom-blocks.

@@ -9,6 +9,21 @@
+    case 'admin/help#custom_block':
+      return 'Allows the creation of custom blocks through the user interface.';
+
+    case 'admin/structure/custom-blocks/types':
+      $output = '<p>This page provides a list of all custom-block types on your site. Each custom-block type can consists of different fields and display settings. From here you can manage the fields on each custom-block type as well as create new custom-block types.</p>';
+      return $output;

Shouldn't these be running through t()?

tim.plunkett’s picture

StatusFileSize
new1.17 KB
new18.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]

Yes

tim.plunkett’s picture

StatusFileSize
new3.94 KB
new18.76 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Missed some spots.

dawehner’s picture

@@ -0,0 +1,118 @@
+    $this->assertTitle(t('Custom blocks | Drupal'));
...
+      $this->assertTitle(t('Edit custom block Antelope | Drupal'));
...
+    $this->assertFieldByXpath('//td', t('Albatross'), "Label found for updated 'Albatross' entity.");
...
+    $this->assertTitle(t('Are you sure you want to delete Albatross? | Drupal'));
...
+    $this->assertNoFieldByXpath('//td', t('Albatross'), "No label found for deleted 'Albatross' entity.");

Please use placeholders ... as we don't want to generate new strings. I guess the same counts for the other lines ....

jessebeach’s picture

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -9,6 +9,21 @@
+function custom_block_help($path, $arg) {
+  switch ($path) {
+    case 'admin/help#custom_block':
+      return t('Allows the creation of custom blocks through the user interface.');
+
+    case 'admin/structure/custom-blocks/types':
+      $output = '<p>' . t('This page provides a list of all custom-block types on your site. Each custom-block type can consists of different fields and display settings. From here you can manage the fields on each custom-block type as well as create new custom-block types.') . '</p>';
+      return $output;
+
+  }
+}

s/can consists/can consist

Consider instead:

"The page lists user-created blocks. Blocks are derived from block types. A block type can consist of different fields and display settings. From the block types secondary tab you can manage these fields as well as create new block types.

tim.plunkett’s picture

StatusFileSize
new5.72 KB
new18.91 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

@dawehner dropping some knowledge about t() in tests.
Thanks @jessebeach, I adjusted the help text.

tim.plunkett’s picture

StatusFileSize
new785 bytes
new18.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,131 pass(es).
[ View ]

ARGH.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

I hope that tim is not 100% killed.

jessebeach’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new70.28 KB
new63.73 KB
jessebeach’s picture

Status:Needs review» Reviewed & tested by the community

Derp, didn't mean to downgrade from RTBC. Testing and reviewed. Kevin O'Leary went over the UI with me in person and indicated approval.

benjy’s picture

The first point I mentioned in #32 still seems to be an issue for me:

To demonstrate from a fresh install what I mean.

  • Add two custom block types
  • Add a custom block from admin/structure/custom-blocks

Expected Behaviour

You arrive back at: admin/structure/custom-blocks

Actual Behaviour

You arrive at: admin/structure/block/list/bartik

tim.plunkett’s picture

That's true in HEAD too. Needs test coverage and further discussion, which is what #2062715: [META] Many UI/UX issues with custom blocks. is for.

tim.plunkett’s picture

StatusFileSize
new61.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,145 pass(es).
[ View ]

@webchick request a combined patch of #2058321: Move the 'place block' UI into the block listing and this issue, here it is.

webchick’s picture

Ok, sat down to review three sites tonight:

1. Just HEAD. #2062715: [META] Many UI/UX issues with custom blocks. covers the current state of that pretty well.
2. HEAD + this issue (#38).
3. HEAD + both patches (#44).

(Note: I tried not to read any of the other reviews so I could go into this fresh. I will probably repeat some of what's above, and will go back after and reconcile this after.)

With this patch on its own, the differences you'll note over the status quo are:

Label of menu item under admin/structure changed from "Custom block types" to "Custom blocks" (I think this is a good change; "Custom block types" looked weird just under "Content types"):

New label for custom blocks.
https://www.evernote.com/shard/s306/sh/1d494ec5-489b-4895-89f6-089baa812...

Under there, the first thing you'll notice is "Basic block" is gone (?!!) (It's actually there, but buried under a "types" sub-tab). Instead you get an empty listing with a grammar error. (this is being looked at in #1850080: Expand entity type labels). On the bright side, the button label is fixed, so yay.

New block listing page, empty by default.
https://www.evernote.com/shard/s306/sh/d048a046-5f15-468d-ae33-aa38e074a...

I worry "Types" will not be found by anyone here. Though, post-review and looking back at previous replies, I can see why this is done. Moving custom blocks under "Content" opens up a huge can of worms (even if I think that's where it belongs ;)), and configuring types is definitely the lesser-used option, so makes sense for it to be buried over the custom block listing itself. We could argue that it's analogous to what we do with Roles on the People page.

---

Ok, let's try adding a block. In HEAD, this requires going back to admin/structure/block, then "Place block," then "Add custom block." Ouch. This is also why we need this patch for the other; if the place blocks UI goes away, the thing that the button's hinged off of no longer exists. With the patch, it's just at the top of the block listing table, same as with nodes. Yay.

In either case, you get a form that looks like this:

Block add form
https://www.evernote.com/shard/s306/sh/5e211e07-ab17-4e16-b4a3-14b3dfb08...

It jumps to an "Add Basic block custom block" form immediately, because there's only one possible type (nodes do this, too). If there were multiple, you'd get a selection here, just as you do on node/add.

I didn't test the multiple block type flow, but looks like benjy did and found issues. Tim argues that those issues are an existing problem in HEAD and therefore shouldn't be dealt with here. I kind of prefer to see it fixed here so we can call this done, but not sure how involved it is.

In both flows, you end up at a listing with your block, and some operations you can do:

Block now listed in the table, can Edit/Delete it.
https://www.evernote.com/shard/s306/sh/1b60080b-af51-4228-8dd1-e280d2a11...

And here, you also are stranded, because the only thing you can do is either "Edit" or "Delete" your block. There's no operation to "Place" the block. This seems like a fairly obvious thing to add. Tim notes that there is no UI for selecting a theme; to me this'd just be a drop-down using the states system. However, since this is a pre-existing issue, and since it might need more detailed UX discussion, it can be a follow-up.

Also of note, hitting "Edit" on the block boots you out of the admin UI and back to Bartik. Wah-wah. This happens in HEAD too, though, so another follow-up.

In the current HEAD UI, you'd go to the place blocks page and see your block there now. With the new patch, same thing except you see it on the block listing in the sidebar:

Block now appears in place block listing.
https://www.evernote.com/shard/s306/sh/9004213e-a7e5-4d1b-b35e-4d0c36b27...

The place block pages look the same, other than HEAD uses vertical tabs and this patch seems to use Details elements, which makes a funky CSS problem when viewing at a narrower width:

Title is incorrect, weird white space next to details elements.
https://www.evernote.com/shard/s306/sh/c9f8ac54-7ceb-4a39-bb06-7a1f76c98...

The CSS issues with narrower widths seem to have been introduced in this patch, so I'd like to see them fixed here if that is correct. If it's not correct (which is possible, since I was mooshing my windows all over the place to take screenshots), then follow-up it is.

Also, knowing Dries, he will want to see that title changed to the action you actually clicked on to get there.

---

Overall, this seems pretty nice, and definitely pushes things forward. None of the problems identified by either me nor others seem like they're critical/major; just normal stuff, all of which already exist in the current code, so I think this can probably be committed once the CSS issue is fixed, pending an answer on whether benjy's issue is easy to fix here, as well as the title change on place blocks form. It might be nice to get catch/alex to look too, though, if they're available.

Code looked fine to me, other than...

+++ b/core/modules/block/custom_block/custom_block.module
@@ -25,6 +40,19 @@ function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {
+      $data['actions']['block/add/o'] = array(
+        '#theme' => 'menu_local_action',
+        '#link' => $item,
+      );

block/add/o? Is that a typo, or what does that mean?

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Tentative needs work for the last paragraph or so in #45.

larowlan’s picture

Yeah thats a typo

Also of note, hitting "Edit" on the block boots you out of the admin UI and back to Bartik. Wah-wah. This happens in HEAD too, though, so another follow-up.

There's an RTBC issue for that - details on the #2062715: [META] Many UI/UX issues with custom blocks.

webchick’s picture

...not anymore, there's not! ;)

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new2.03 KB
new18.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]

@benjy's thing is not easily fixed, as we have no real test coverage for the multiple block type workflow.

Here's the fix, that was a weird typo!

I've also converted the local action I was adding now that #2050227: Add local action plugin deriver to use YAML discovery for static definitions is in.

tim.plunkett’s picture

Note: not fixing the weird string in the empty table until #1850080: Expand entity type labels is in. The string is from EntityListController::render(), and is broken for every entity type until it is fixed for every entity type.

dawehner’s picture

@@ -47,7 +47,7 @@ function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {
-      $data['actions']['block/add/o'] = array(
+      $data['actions']['block/add'] = array(

Was that a vim issue?

@@ -9,6 +9,21 @@
+  switch ($path) {
+    case 'admin/help#custom_block':
+      return t('Allows the creation of custom blocks through the user interface.');
...
+    case 'admin/structure/custom-blocks/types':

Maybe also a help message for admin/structure/custom-blocks would be helpful.

@@ -0,0 +1,50 @@
+  public function buildHeader() {
+    $header = parent::buildHeader();
+    unset($header['id']);
+    return $header;
+  }

It would be great to have "block description or just description" instead of "label" as the table header.

tim.plunkett’s picture

StatusFileSize
new2.59 KB
new18.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,094 pass(es).
[ View ]

Yes, that makes sense. Thanks @dawehner.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you very much!

alexpott’s picture

StatusFileSize
new30.48 KB

lonely-block-page-2.png

So the custom block help page looks at bit lonely. The help on admin/help/block is actually more useful. However whilst this page is actually more help the link it gives for adding custom blocks is admin/structure/block/list/bartik/add/custom_blocks - should this not be admin/structure/custom-blocks now?

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockListController.phpundefined
@@ -0,0 +1,51 @@
+class CustomBlockListController extends EntityListController {
...
+  public function buildHeader() {
+    $header = parent::buildHeader();
+    $header['label'] = t('Block description');
+    unset($header['id']);
+    return $header;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildRow(EntityInterface $entity) {
+    $row = parent::buildRow($entity);
+    unset($row['id']);
+    return $row;
+  }

Are we sure we want to the parent and then unset something it did we don't like pattern here. These methods won't really get any longer either.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockListTest.phpundefined
@@ -0,0 +1,122 @@
+    $this->assertLink($link_text);
+    $this->clickLink($link_text);

Is it necessary to both assetLink and clickLink?

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

In discussing with @timplunkett in IRC he pointed to #2062761: Update hook_help() for custom_block modules to improve the hook_help() implementation.

And we going to cover the inheritance issue with parent::buildHeader() and parent::buildRow() in an issue to correct all config entities. I think the pattern of calling parent and unsetting stuff is wasteful and will lead to unexpected bug in the future. Plus calling parent:: blindly leads is an anitpattern - inheritance is the tightest form of coupling.

And the other comment about the test is so minor I'm happy to ignore.

Committed a0d3030 and pushed to 8.x. Thanks!

tim.plunkett’s picture

Opened #2064591: Missing test coverage for multiple custom block types to stop introducing more problems.

xjm’s picture

This patch still introduced a major usability issue, as described in #42, that hasn't yet been fixed. I don't see any followup issue for it here, so I filed: #2068053: Custom block creation form redirect is inconsistent

tim.plunkett’s picture

xjm’s picture

No, it wasn't.... try it... watch the video. :)

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.