Move the 'place block' UI into the block listing

@@ -0,0 +1,33 @@
+ * Contains \Drupal\custom_block\Plugin\Menu\LocalAction\CustomBlockAddLocalActionDeriver.

At least from my observations the derivative classes never ended up in the actual plugin folders?

@@ -0,0 +1,33 @@
+class CustomBlockAddLocalActionDeriver extends DerivativeBase {
...
+  public function getDerivativeDefinitions(array $base_plugin_definition) {
...
+    $this->derivatives = array($derivative);
+    return parent::getDerivativeDefinitions($base_plugin_definition);

I thought we decided to use an alter hook for now?

@@ -0,0 +1,60 @@
+    function hidePackageDetails(index, element) {
...
+    function filterModuleList (e) {
...
+      function showModuleRow (index, block) {

i guess some of them should get some docs?

@@ -52,7 +97,7 @@ public function load() {
-   * Overrides \Drupal\Core\Entity\EntityListController::render().
+   * {@inheritdoc}

@@ -62,27 +107,31 @@ public function render($theme = NULL) {
-   * Implements \Drupal\Core\Form\FormInterface::getFormID().
+   * {@inheritdoc}
...
-   * Implements \Drupal\Core\Form\FormInterface::buildForm().
-   *
-   * Form constructor for the main block administration form.
+   * {@inheritdoc}

@@ -253,19 +372,17 @@ public function getOperations(EntityInterface $entity) {
-   * Implements \Drupal\Core\Form\FormInterface::validateForm().
+   * {@inheritdoc}
...
-   * Implements \Drupal\Core\Form\FormInterface::submitForm().
-   *
-   * Form submission handler for the main block administration form.
+   * {@inheritdoc}

Slightly out of scope.

@@ -210,25 +259,95 @@ public function buildForm(array $form, array &$form_state) {
+          'data-dialog-options' => json_encode(array(
+            'width' => 700,
+          )),

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/BasicTest.php
@@ -122,7 +122,7 @@ function testViewsWizardAndListing() {

just wondering whether Json::encode should be used instead of consistency (yes there is no special sigh here).

Oeeh, nice! Great work tim!

We really gotta fix Seven's design in regards to sidebars. But that isn't part of this issue.

This is an essential part of #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout, so bumping to critical.

block.admin.js has only one thing to fix:
if (query.length == 0) {

this does not pass JSHint validation, it should be
if (query.length === 0) {

Other than that, all good for me :)

EDIT: moved comment over to #2055853-6: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout, since it concerned the design, not the implementation.

I will say, even as someone who has a pretty good idea where all of these blocks come from, I actually do like the open version better. Especially since the list of blocks can change randomly based on other configuration in the site (modules enabled/disabled, views created, etc.).

There is a small bug, this might be dialog related though. Once I add a block and it exits the modal, onto the block page it trows me out of the overlay. The modal dialog is also strangely attached to the top of the page, rather than a little more vertically centered.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.phpundefined
@@ -18,6 +18,7 @@
+ *   category = "menu",

Pretty sure this should be translatable. I don't care if it's in a follow up, but it needs doing.

I'm probably not the person to RTBC this, but I will say I definitely support this. As much as I like the Plugin UI code, it doesn't belong in core (yet), and it was written too early in the life cycle of D8. Having used this new UI (at various stages) I like where this is going. FWIW I'm very ++ here.

Eclipse

@@ -1,60 +0,0 @@
-class BlockLibrarySearchTest extends WebTestBase {

What is the replacement for that test, I can't find it ...

Just to write done from yesterday night, there should be tests for the generated blocks list on the right side.

This patch looks great. Love the diffstat, once again.

I tested the patch manually with @Dries, @webchick, and @effulgentsia. The current version has no usability regressions over what's currently in HEAD, and two clear improvements:

• It's now obvious how to find blocks to add to the page, and obvious that adding them is still a part of the admin/structure/blocks workflow, which is one of the biggest problems with what's in HEAD.
• The new block filtering replaces the odd machine-name-based autocomplete plus page load with a live filter of human-readable labels.

I have just three questions:

1. +++ b/core/modules/block/custom_block/custom_block.moduleundefined
   @@ -9,20 +9,14 @@
    /**
   - * Implements hook_menu_local_tasks().
   + * Implements hook_menu_local_actions_alter().
     */
   -function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {
   -  // Add the "Add custom block" action link to the theme-specific block library
   -  // listing page.
   -  // @todo This should just be $root_path == 'admin/structure/block/list/%/add'
   -  //   but block_menu() registers static router paths instead of dynamic ones.
   -  if (preg_match('@^admin/structure/block/list/(.*)/add$@', $root_path)) {
   -    $item = menu_get_item('block/add');
   -    if ($item['access']) {
   -      $data['actions']['block/add'] = array(
   -        '#theme' => 'menu_local_action',
   -        '#link' => $item,
   -      );
   +function custom_block_menu_local_actions_alter(&$actions) {
   +  if (isset($actions['custom_block_add_action'])) {
   +    foreach (list_themes() as $theme => $theme_info) {
   +      if ($theme_info->status) {
   +        $actions['custom_block_add_action']['appears_on'][] = "block_admin_display.$theme";
   
   @@ -93,21 +87,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',
   -      ),
   -    ),
   -  );
   -}
   
   +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Menu/LocalAction/CustomBlockAddLocalAction.phpundefined
   @@ -0,0 +1,24 @@
   +<?php
   +
   +/**
   + * @file
   + * Contains \Drupal\custom_block\Plugin\Menu\LocalAction\CustomBlockAddLocalAction.
   + */
   +
   +namespace Drupal\custom_block\Plugin\Menu\LocalAction;
   +
   +use Drupal\Core\Annotation\Menu\LocalAction;
   +use Drupal\Core\Annotation\Translation;
   +use Drupal\Core\Menu\LocalActionBase;
   +
   +/**
   + * @LocalAction(
   + *   id = "custom_block_add_action",
   + *   route_name = "custom_block_add_page",
   + *   title = @Translation("Add custom block"),
   + *   appears_on = {"block_admin_display"}
   + * )
   + */
   +class CustomBlockAddLocalAction extends LocalActionBase {
   +
   

   So we're adding the custom block local action back on this page since we got rid of the other form it was on and now there's no place block local action to confuse it with. That makes sense.

   It seems a little odd that custom block is altering its own local actions here. I can see that we have to add the local action on a per-theme basis, rather than just once, but this is a lot of code to do just that.

   Maybe this is just a side effect of me not understanding why local actions have to be special and different and have all this stuff around them, and therefore out of scope, but I think this will probably come up in a maintainer review and so want to make sure I understand.

2. +++ /dev/nullundefined
   @@ -1,60 +0,0 @@
   -class BlockLibrarySearchTest extends WebTestBase {
   

   So the equivalent of this test for the new UI is purely a JS test; let's add a followup for that for FAT? @nod_ / @jessebeach, thoughts?

3. The added test coverage looks mostly sufficient to me, but one thing seems missing: It seems like we need to have test coverage for the +My Blockname link taking you into the modal, and then the submission of that form returning you to admin/structure/block with the newly-placed block added. (WebTestBase::drupalPlaceBlock() deliberately creates blocks through the API rather than the UI.) I don't think there was anything in HEAD covering this previously; it was one of the gaps in test coverage related to the now-removed plugin UI stuff. Thoughts?

So sounds like two FAT test scenarios to add, then: one for the filtering and one for the add link and modal.

Are there existing local actions issues we can reference in the summary? Thanks!

+++ b/core/modules/block/block.routing.ymlundefined
@@ -26,10 +26,3 @@ block_admin_add:
-block_autocomplete:

+++ b/core/modules/block/custom_block/custom_block.moduleundefined
@@ -9,20 +9,14 @@
-function custom_block_menu_local_tasks(&$data, $router_item, $root_path) {

@@ -93,21 +87,6 @@ function custom_block_menu() {
-function custom_block_local_actions() {

@@ -221,27 +200,6 @@ function custom_block_add_body_field($block_type_id, $label = 'Block body') {
-function custom_block_form_block_plugin_ui_alter(&$form, $form_state) {

+++ b/core/modules/block/lib/Drupal/block/BlockListController.phpundefined
@@ -7,15 +7,20 @@
-class BlockListController extends ConfigEntityListController implements FormInterface {
+class BlockListController extends ConfigEntityListController implements FormInterface, EntityControllerInterface {

+++ /dev/nullundefined
@@ -1,78 +0,0 @@
-class BlockAutocompleteController implements ControllerInterface {

+++ /dev/nullundefined
@@ -1,157 +0,0 @@
-class PlaceBlocksForm implements FormInterface, ControllerInterface {

Closest things to API changes in the patch. BlockListController now implements EntityListControllerInterface and so has to add a couple methods, which would affect anyone subclassing this form for whatever reason. Otherwise, we're just changing the ArrayPI structure of the admin/structure/blocks form (obviously) and removing:

• The interim autocomplete route and its controller.
• Some hook implementations.
• The separate form controller and theming for the old "place blocks" page, which no longer exists.

Oh, and after looking at the interdiff in #9, I agree that the alter for the local actions is a step up over using derivatives for it. ;) Let's file a followup though to discuss whether there's a less insane way to handle stuff like that.

This patch is good to go from my perspective, but I'll give @dawehner a chance to weigh in on it again too.

Oh, and after looking at the interdiff in #9, I agree that the alter for the local actions is a step up over using derivatives for it. ;) Let's file a followup though to discuss whether there's a less insane way to handle stuff like that.

On the longrun we might want to have a dynamic plugin registration much like hook_entity_info() ...

Opened a follow up: #2061777: Provide dynamic registration of local_action plugins

The interdiff of #25 looks fine, so I am fine with the patch. Given #2058321-11: Move the 'place block' UI into the block listing I assume that the JS is fine.

Updated issue summary.

Updated issue summary.

I also tested this and it looks good. However there are a few styling issues on both the iPhone and iPad. Screenshots attached.

@@ -61,7 +61,7 @@ function block_help($path, $arg) {
-        $output .= '<dd>' . t('Users with the <em>Administer blocks</em> permission can <a href="@block-add">add custom blocks</a>, 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'), '@block-add' => url('admin/structure/block/list/' . config('system.theme')->get('default') . '/add/custom_blocks'))) . '</dd>';

How come we removed the link to "add custom blocks"? Couldn't we link to this to: block/add?

Looks good now. Opened a follow up for iPhone issue which also appears at admin/content. #2061873: Toolbar and Page pushed over at admin/content on iPhone

Just tested this on an Android. You can't press the save blocks button in the modal because when you scroll to the button, it jumps back to the top of the modal before you can press the button. Will file a follow-up on Monday as that isn't this issue's fault

+/**
+ * @LocalAction(
+ *   id = "custom_block_add_action",
+ *   route_name = "custom_block_add_page",
+ *   title = @Translation("Add custom block"),
+ *   appears_on = {"block_admin_display"}
+ * )
+ */
+class CustomBlockAddLocalAction extends LocalActionBase

Out of scope: does anyone else think this is overkill just for a local action link. Seems Like yml files would suit better.

   }
-    list($base, $derivative) = explode(':', $plugin_id);
-    if ($base !== 'custom_block') {
-      continue;
-    }
-    $custom_block = entity_load_by_uuid('custom_block', $derivative);
-    $row['1']['data']['#links']['edit'] = array(
-      'title' => t('Edit'),
-      'href' => 'block/' . $custom_block->id(),
-    );
-  

I can't see where this is replaced. But I'm reviewing on my mobile without dreditor so might have missed it. This was the only link to edit the custom block content entity other than contextual links, which I'm not sure have a non Javascript fallback.

This is the block listing page. There is no other page.

Which means we need admin/content/block listing using views?

This issue is (alas) currently blocked on something like #2062439: Provide listing of custom block entities.

Per the docs gate, this issue needs to update block_help() for admin/structure/block.

Updated issue summary.

There was no reference to the old place blocks in block_help() so assigning to xjm for feedback on expectations regarding hook_help() here.
If nothing, then RTBC.

1. At admin/structure/custom-blocks the empty listing text could be improved however I think that's out of scope here.

2. Also, this confused the hell out of me.

• Go to admin/structure/block
• Add a custom block
• End up at: admin/structure/block/list/bartik
• Where's the "Add custom block" button gone?

Do we want a follow up for 2?

I understand the combination of this being RTBC and the review in #2055853-6: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout makes #1888702: Use configuration selection instead of derivatives for some blocks a bug, for those block derivates that could have large numbers. Will update the issue over there with what it currently does ASAP and which blocks should be derivates and which shouldn't. I guess most others will stay.

Who wants to change the configuration/derivative issue to a (major?) bug? :)

New patch with a slight update to block_help() per #45.

Probably makes sense for Dries to sign off on this interim step, since he had some pretty serious reservations about it back a week or so ago.

I'm ok with this being committed as an interim step. There seems to be consensus that this interim step is a big step forward. Just keep in mind that the final solution could look quite a bit different (see #2055853-22: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout) as there are still major UX issues with this interim solution.

Committed this 8.x. Thank you.

#2056513: Remove PluginUI subsystem, provide block plugins UI as one-off might need a re-roll because of this; not sure yet.

Given that this is an interim step, I don't think we need to write up a change notification just yet. It is probably good to track it already? I'm leaving this as critical.

Nice! This is really a significant interim step. I'm loving the steady improvement this UI is going through. Small steps let us lock in improvements.