Updated: Comment #32

Problem/Motivation

See #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout

Proposed resolution

Move the list of candidate blocks into the sidebar of the block listing.

Remaining tasks

None blocking commit. Followups should be filed for adding frontend tests for the list filtering functionality and the block add/modal workflow.

User interface changes

See #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout.

API changes

  • BlockListController now implements EntityListControllerInterface.
  • The interim autocomplete route and its controller are removed.
  • Some hook implementations are removed.
  • The separate form controller and theming for the old "place blocks" page are removed, because the page no longer exists.
  • The render array structure of admin/structure/blocks is changed.

#2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout
#2056513: Remove PluginUI subsystem, provide block plugins UI as one-off
#2050227: Add local action plugin deriver to use YAML discovery for static definitions
#2061777: Provide dynamic registration of local_action plugins
#2029731: Improve help for blocks module

Files: 
CommentFileSizeAuthor
#54 blocks-ui.interdiff.txt1.6 KBpameeela
#54 block-2058321-54.patch46.97 KBpameeela
PASSED: [[SimpleTest]]: [MySQL] 58,164 pass(es).
[ View ]
#51 block-2058321-51.patch45.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,809 pass(es).
[ View ]
#50 block-2058321-50.patch45.43 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,822 pass(es).
[ View ]
#49 interdiff.txt917 bytestim.plunkett
#49 block-2058321-49.patch43.61 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#46 interdiff.txt1.53 KBtim.plunkett
#46 block-2058321-46.patch43.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,794 pass(es).
[ View ]
#36 block-2058321-36.patch44.01 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,740 pass(es).
[ View ]
#36 interdiff.txt411 bytestim.plunkett
#35 block-2058321-35.patch44.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#35 interdiff.txt3.17 KBtim.plunkett
#33 iPad Retina.JPG402.59 KBbenjy
#33 iPhone4.PNG80.72 KBbenjy
#25 block-2058321-25.patch44.09 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]
#25 interdiff.txt2.53 KBtim.plunkett
#17 block-2058321-17a.patch42.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,804 pass(es).
[ View ]
#17 block-2058321-17b.patch42.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,572 pass(es).
[ View ]
#14 block-2058321-14a.patch56.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,581 pass(es).
[ View ]
#14 block-2058321-14b.patch56.82 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,908 pass(es).
[ View ]
#14 a-b-diff.patch629 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch a-b-diff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 interdiff.txt1.29 KBtim.plunkett
#14 14a-collapsed.png139.95 KBtim.plunkett
#14 14b-open.png147.31 KBtim.plunkett
#13 interdiff.txt824 bytestim.plunkett
#13 block-2058321-13.patch56.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,931 pass(es).
[ View ]
#9 block-2058321-9.patch41.97 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,817 pass(es).
[ View ]
#9 interdiff.txt9.18 KBtim.plunkett
#3 block-2058321-3a-details.png202.54 KBtim.plunkett
#3 block-2058321-3b-filtering.png180.64 KBtim.plunkett
#3 block-2058321-3c-modal.png219.21 KBtim.plunkett
#2 block-2058321-2.patch44.93 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]
#1 block-2058321-1.patch19.33 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs work
StatusFileSize
new19.33 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Will set to NR when #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off goes in.
Tests will fail even then, this is not ready for review yet.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new44.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,854 pass(es).
[ View ]

Blockers have gone in!
Screenshots to come later today.

tim.plunkett’s picture

StatusFileSize
new219.21 KB
new180.64 KB
new202.54 KB

s/later today/right now/

Expanding the block category details elements
Expanding the block category details elements

Filtering the available blocks
Filtering the available blocks

Block add form in a modal
Block add form in a modal

dawehner’s picture

@@ -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).

tim.plunkett’s picture

I copied from MockMenuBlockDeriver, but that looks like it might be one of the only ones :) But it doesn't matter because:

Right, I forgot to switch to an alter hook. Will do.

This JS is borrowed directly from system.modules.js, which is borrowed from simpletest.js, which I believe nod_ wrote. So I'll leave it to him.

Yes, I'll just revert those changes.

This is also copied directly, from ConfigSync in this case. I don't think it matters.

Bojhan’s picture

Oeeh, nice! Great work tim!

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

xjm’s picture

tim.plunkett’s picture

StatusFileSize
new9.18 KB
new41.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,817 pass(es).
[ View ]

This addresses @dawehner's review, and fixes a CSS bug on small screens.

tim.plunkett’s picture

Issue tags:+JavaScript

Summoning the JS team, block.admin.js is an adaptation of system.modules.js, which was copied from simpletest.js

nod_’s picture

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 :)

webchick’s picture

Issue tags:+Usabilty
tim.plunkett’s picture

StatusFileSize
new56.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,931 pass(es).
[ View ]
new824 bytes

jbeach pointed out a flaw in showing a details element with jQuery, which is present on the modules page (where I copied my code from), see #2060573: Standardized filter/search behavior; surface filter matches regardless of their containing box's open state; for that.

This fixes that bug, and addresses #11.

tim.plunkett’s picture

StatusFileSize
new147.31 KB
new139.95 KB
new1.29 KB
new629 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch a-b-diff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new56.82 KB
PASSED: [[SimpleTest]]: [MySQL] 57,908 pass(es).
[ View ]
new56.85 KB
PASSED: [[SimpleTest]]: [MySQL] 57,581 pass(es).
[ View ]

This includes further improvements from that JS issue (which is now RTBC).

I've attached two versions of this patch. One has the categories initially collapsed, like all the previous patches. The other has them initially open.

Collapsed:
14a-collapsed.png
Open:
14b-open.png

webchick’s picture

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.).

tim.plunkett’s picture

StatusFileSize
new42.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,572 pass(es).
[ View ]
new42.22 KB
PASSED: [[SimpleTest]]: [MySQL] 57,804 pass(es).
[ View ]

Ugh, that was a bad rebase, it accidentally reverted the views block patch from #2057831: Exposed filter blocks do not work anymore.

Interdiffs were correct.

Bojhan’s picture

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.

EclipseGc’s picture

+++ 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

tim.plunkett’s picture

tim.plunkett’s picture

Title:Move the "place block" UI into the block listing» Move the 'place block' UI into the block listing

Between @Bojhan, @webchick, and @Dries, we seem to have consensus on going with 17B (the expanded by default).
I personally liked collapsed by default, but @Bojhan's points in #2055853-9: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout are convincing. Also, it will help us focus on @Berdir's #1888702: Use configuration selection instead of derivatives for some blocks for improvements to faulty block derivatives.

So, any further reviews, please focus on 17B.

(retitling to prevent a bad commit message)

dawehner’s picture

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

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

tim.plunkett’s picture

We've removed the autocomplete controller completely. We now have JS-only filtering of block labels, instead of autocomplete of block plugin IDs.

dawehner’s picture

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

tim.plunkett’s picture

StatusFileSize
new2.53 KB
new44.09 KB
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]

Good idea. This test checks the category structure, and then proves you can move them around with an alter hook. Should be sufficient.

xjm’s picture

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?
tim.plunkett’s picture

1) Yes, this alter was requested by @dawehner, I previously abused derivatives to do it (see the interdiff in #9 for the switch).
We have no standard approach to dynamic paths for a local action, and the consensus (pwolanin, dawehner and I) landed on alter.

3) We can't test that it takes you into a modal. It's just unpossible. All of our dedicated modal tests are completely faking it by setting the test content to a bunch of JSON and xpathing it. So if we try to test this, it will just use the regular paths as though JS was disabled. \Drupal\block\Tests\BlockTest already covers this.

xjm’s picture

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!

xjm’s picture

+++ 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.
xjm’s picture

Issue tags:+Needs followup

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.

tim.plunkett’s picture

\Drupal\filter\Plugin\Menu\AddFilterFormatLocalAction is an example of another. grep -nr @LocalAction * will show you them all

xjm meant she was looking for a link to #2050227: Add local action plugin deriver to use YAML discovery for static definitions

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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.

dawehner’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

benjy’s picture

StatusFileSize
new80.72 KB
new402.59 KB

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?

tim.plunkett’s picture

Those screenshots are wrong. You didn't properly clear either the Drupal or browser cache. I know this because the styling looks wrong, but also because you have a "Place blocks" local action.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.17 KB
new44.24 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Talked through this with @benjy, realized I was missing some media queries.

#2061863: Make two column node CSS reusable is a follow-up to stop copying this CSS everywhere.
#2056173: Regression: Two-column layout is broken by vertical toolbar is a bug well be seeing here now, since its the same CSS.

tim.plunkett’s picture

StatusFileSize
new411 bytes
new44.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,740 pass(es).
[ View ]

Included one line from debugging, didn't mean to.

benjy’s picture

Status:Needs review» Reviewed & tested by the community

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

larowlan’s picture

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

larowlan’s picture

+/**
+ * @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.

tim.plunkett’s picture

@larowlan your first question was asked in #29 and answered in #31

As far as the second part, we no longer have an operations dropbutton, because the old "Place blocks" operation is now the link with the block plugin title. This was by design.

Can't they just get to the custom block content entity edit from the listing page? Also, it could be a local task on the block configure form possibly.

Either way, that's a question for the parent issue.

larowlan’s picture

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

larowlan’s picture

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

tim.plunkett’s picture

Custom block needs its own listing. And that could use views. This cannot because we don't have draggableviews in core. (Please continue this discussion on #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout or open a follow-up)

xjm’s picture

Status:Reviewed & tested by the community» Postponed

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

xjm’s picture

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

xjm’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

Status:Postponed» Needs review
StatusFileSize
new43.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,794 pass(es).
[ View ]
new1.53 KB

Blocker went in.

I don't know what else it should say. Suggestions welcome.

Converted the local action to YAML.

larowlan’s picture

Assigned:tim.plunkett» xjm
Status:Needs review» Reviewed & tested by the community

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.

benjy’s picture

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?

tim.plunkett’s picture

StatusFileSize
new43.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new917 bytes

My mistake! The change in #46 wasn't done all the way.

#48.1 is blocked by #1850080: Expand entity type labels

tim.plunkett’s picture

StatusFileSize
new45.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,822 pass(es).
[ View ]

ARGH. Wrong patch, correct interdiff.

tim.plunkett’s picture

StatusFileSize
new45.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,809 pass(es).
[ View ]
Berdir’s picture

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? :)

tim.plunkett’s picture

If this goes in, I will.

pameeela’s picture

StatusFileSize
new46.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,164 pass(es).
[ View ]
new1.6 KB

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

tim.plunkett’s picture

Assigned:xjm» Unassigned

Thank you so much @pameeela! Now we don't need to wait for xjm.

webchick’s picture

Assigned:Unassigned» Dries

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.

Dries’s picture

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.

Dries’s picture

Assigned:Dries» Unassigned
Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs change record

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.

jessebeach’s picture

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.

tim.plunkett’s picture

Status:Needs work» Fixed
Issue tags:-Needs change record

This only removes a form alter, and the form and controller we just moved in #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off, which was never documented in the first place.
Any real change notice will come from the parent issue.

This still needs follow-ups for adding FAT tests, but nothing left to do in core.

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.