When placing a block in the core block UI, it lists all available blocks in a list. While it shows the category, it lists all the blocks from all categories together. This makes it difficult to "drill down" to the block you actually want -- especially after enabling a few contrib modules, creating a few views or creating a few content blocks.

Instead, we should show vertical tabs with a tab for each category, and only show the blocks for a category after the user has clicked the appropriate tab. This is similar to the "Add content" dialog in Panels in D7.

This way a user can say "Ok, I want to place a form, so I'll click 'Forms' to find the one I want", for example.

Ideally, this should be implemented as a reusable class that we can use in Panels and to override the core block placement dialog.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek created an issue. See original summary.

dsnopek’s picture

Issue summary: View changes
tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.93 KB

This doesn't actually modify the UI yet, but it takes over the controller and just calls parent:: for now.

dsnopek’s picture

Component: Code » CTools Blocks
EclipseGc’s picture

Status: Needs review » Needs work

Ok, what's there looks good, but obviously doesn't do anything yet, so NWing it.

Eclipse

EclipseGc’s picture

FileSize
14.26 KB
7.36 KB

Not interdiffing this considering the minimal-ness of the original patch.

All sorts of issues here, so let's consider this a mostly working mock-up for the time being. Issues that have been discussed in other forums of communication include:

  1. Ajax loading of the tabs instead of always loading all content. This is of great interest because it has a number of meaningful consequences. If we don't do this, it make panopoly_magic's rendering approach unlikely to ever work. It also complicates the search if we do move forward with it and relegates it to a definite ajax call.
  2. 50% Search, 50% browse. This was pointed out to me by DyanneNova, and resonates with my own opinions about this particular UI. Core has a search for blocks already, and I find myself using it a lot, so for me, a search feature in this UI that encompasses the entire scope of all available blocks is a MUST.
  3. I wrote the JS. Which is to say, it's not my forte. What I found was that the same raw js search operation that core uses for this UI is unlikely to work for us since I'm duplicating DOM elements and I need to Drupal.attachBehaviors() to open those elements in modals. That makes for really rough browser performance because we're doing it onkeyup. I commented out that particular line with this same explanation there. Ideally this would be a lot easier in an ajax request because we wouldn't be dynamically updating the DOM as often. I suspect we'll need to react onblur of the filter element. Open to any other thoughts others might have.
  4. UI Consistency is favorable. We need to consider this (or other solutions) as they relate to panels and IPE. I think what I've got in this patch is already a really big improvement as compared to what core does currently. The goal would be for contrib (panels, etc) to be able to leverage this as well though so let's just be conscious of that.
  5. I probably over did it a tad. Specifically overriding the block list builder so that I could increase the width of the modal was likely unnecessary, but made it feel better for me. Changes like that should likely end up in the ctools_block module (when I make it).

I've included the image here as well since it won't be available in the patch. Place it in the images/blocks directory. Let me know what the thoughts are.

Eclipse

japerry’s picture

Interesting, by default, custom created blocks (which did appear in the core blocks system) do not appear with this patch. I suppose this could be considered a bug or a feature! See #2626942: Creating Custom Blocks that are not re-usable for more info.

EclipseGc’s picture

FileSize
25.06 KB

Ok, no interdiff because it's REALLY different. I ejected jquery.ui.tabs in favor of building something custom. This is because tabs was expecting full page renders that it would then replace the contents of a div with. This seemed ugly in general, and also likely to fail as soon as we were rendering blocks in the list since some have attachments and that particular methodology would be prone to failure in that situation without being REALLY heavy handed.

The result is a set of code I a.) like better and b.) am in complete control of (perhaps those two things are related? I can't tell at this vantage point).

I've introduced a block_ui plugin type. This is maybe 50% baked at the moment and needs some more love. Likewise the Icons implementation of that plugin contains a lot of code that can perhaps be abstracted. We need to do due diligence on this and make sure that I didn't force some sort of dependency on what my expectations for the content of those "tabs" were. There are a number of known bugs in this code:

  1. NULL search results don't give any sort of feedback currently
  2. Images should probably be clickable
  3. The filter form is hard-coding its response, and we probably need to be injecting the plugin and letting it do the work so that the form is agnostic of the style that's returned.

I think this is a big step in the right direction, and a bit of the blocks code we've introduce here will ultimately migrate into ctools_blocks. Next steps are to fix the known problems and then I want to introduce a new plugin type for custom block types. This would ultimately allow us to do things like add "Content blocks" or introduce a custom plugin for building new views (crazy but just an example) etc etc. It's a plugin as a platform for handling non-standard block types (which core posses two of, content blocks and views).

Same image from the previous patch still applies. This patch should now apply cleanly.

Eclipse

EclipseGc’s picture

FileSize
27.75 KB
20.44 KB

Ok, I reworked things to make them saner and more extensible. A lot of cleanup between the Icons class and the BlockUIBase could still happen, but this is beginning to move in the correct direction. The filter form now expects a BlockUIInterface plugin to be injected into it, it then farms out the render process of the results it finds to the plugin, so it's pretty generic at this point. The plugins can expect their urls to come across the configuration now, likewise contexts come across config which is something we've done elsewhere in core. Long term, I intend on adding a filter for removing blocks from the list of available blocks based upon a block profile. Also, the "category" listing might should be a trait+interface or something. I'm still debating.

Give this a go, let me know what you think.

Eclipse

samuel.mortenson’s picture

Just had a chance to review this, noticed a few things:

  1. When rebuilding caches I get the error "Strict warning: Declaration of Drupal\ctools\Controller\BlockLibraryController::listBlocks() should be compatible with Drupal\block\Controller\BlockLibraryController::listBlocks(Symfony\Component\HttpFoundation\Request $request, $theme)". I'm guessing this can be solved by chaning the order of arguments in BlockCategoryController::listBlocks()
  2. When I click "Place block" at /admin/structure/block, it seems to load OK but in the console I get this error: "ResponseText: Recoverable fatal error: Argument 1 passed to Drupal\Core\Render\MainContent\AjaxRenderer::renderResponse() must be of the type array, null given".
  3. I'm missing the "images/blocks/no-icon.png " image, I think that's as simple as making the patches using the --binary flag (if you're using git diff).
  4. The search seems to be working well! I think we'll want a timer/count that submits the search form every few keypresses, but I can work on that JS.

Functionally this seems good so far! Should we start work/issues in tandem with this to make sure that other UIs that list blocks (Page Manager's Block Display, Panels's Variants, etc.) can use this as well?

mpotter’s picture

Is there a way to add a "category" for creating new instances of custom block types? Sort of like the static content widgets that we used to have at the bottom of the Add Content form in D7. Right now you can add new block instances from the Blocks page and from the IPE (Create Content), but not from the Page Manager UI. So in terms of consolidating the UIs this would be an important piece not to miss.

So maybe a tab called "New Block Content" or something like that. This is different than just selecting the Blocks category for an existing custom block instance.

sylus’s picture

Status: Needs work » Needs review
FileSize
12.52 KB
39.4 KB

I am just rerolling the patch which won't apply to latest dev anymore (services.yml). I also took care of the first 3 issues that samuel.mortenson noticed.

1) I just moved $plugin_id into the function as isn't an expected param, not sure how we would get variations though.
2) This was because in some cases when there is no string we don't sent a response back.
3) Added the picture in patch.
4) Adjusted services.yml (not in interdiff).

tkoleary’s picture

  1. +++ b/css/block-ui.css
    @@ -0,0 +1,48 @@
    +.ctools-tabs {
    

    Why are we re-inventing vertical tabs here when we have it in core? Is there something special that this version provides that core does not?

  2. +++ b/ctools.module
    @@ -62,3 +77,24 @@ function ctools_condition_info_alter(&$definitions) {
    +    if (empty($definition['icon']) || !file_exists($definition['icon'])) {
    

    Whenever we have ask contrib modules to provide an icon they never. Let's stop doing this. We should either use phantom.js to screenshot the blocks, provide a library of re-usable icons, or just don't use icons.

  3. +++ b/ctools.module
    @@ -62,3 +77,24 @@ function ctools_condition_info_alter(&$definitions) {
    +      $definitions[$plugin_id]['icon'] = '/' . $module_dir . '/images/blocks/no-icon.png';
    

    The default icon should be an SVG, and it should also look good :)

darrenwh’s picture

Status: Needs review » Needs work

A few doc blocks missing:

  1. +++ b/src/Controller/BlockCategoryController.php
    @@ -0,0 +1,67 @@
    +  public function __construct(BlockUIManager $manager, ContextRepositoryInterface $context_repository) {
    

    Missing function doc block

  2. +++ b/src/Controller/BlockCategoryController.php
    @@ -0,0 +1,67 @@
    +
    

    Missing function doc block

  3. +++ b/src/Controller/BlockCategoryController.php
    @@ -0,0 +1,67 @@
    +  public function listBlocks($category, $theme, Request $request) {
    

    Missing method doc block

  4. +++ b/src/Form/BlockSearchFilter.php
    @@ -0,0 +1,102 @@
    +  public static function create(ContainerInterface $container) {
    

    Missing function doc block

  5. +++ b/src/Form/BlockSearchFilter.php
    @@ -0,0 +1,102 @@
    +  public function __construct(BlockManagerInterface $manager) {
    

    Missing function doc block

  6. +++ b/src/Form/BlockSearchFilter.php
    @@ -0,0 +1,102 @@
    +  public function buildForm(array $form, FormStateInterface $form_state, BlockUIInterface $plugin = NULL) {
    

    Missing function doc block

  7. +++ b/src/Form/BlockSearchFilter.php
    @@ -0,0 +1,102 @@
    +  public function submitForm(array &$form, FormStateInterface $form_state) {}
    

    Missing function doc block

  8. +++ b/src/Form/BlockSearchFilter.php
    @@ -0,0 +1,102 @@
    +  public function search(array &$form, FormStateInterface $form_state) {
    

    Missing function doc block

EclipseGc’s picture

Why are we re-inventing vertical tabs here when we have it in core? Is there something special that this version provides that core does not?

Yeah, so fair question. I'm open to contribution here, but here are my basic reasons:

  1. VT doesn't actually visualize like this.
  2. VT typical preloads all the results (perhaps ajax loading is a feature of it with which I am unfamiliar?)
  3. Searching was yet another thing I'd never seen in VT
  4. Last but DEFINITELY not least, I suck with JS, so whatever I could write that would work and was as minimal as possible is what I gave direct preference to. If JS savvy devs which to dig in here, I am all sorts of on-board.

Whenever we have ask contrib modules to provide an icon they never. Let's stop doing this. We should either use phantom.js to screenshot the blocks, provide a library of re-usable icons, or just don't use icons.

True, they do not, which is why I hard-coded a default that can be overridden (I think... been a while since I wrote this code.) I am NOT opposed to any of your other suggestions. They just weren't on the short list of POC.

The default icon should be an SVG, and it should also look good :)

Again, agreed, see my previous comments about shortlists and POC :-D

Eclipse