This issue is blocked by #2905922: Implementation issue for Layout Builder.

Problem/Motivation

Layout Builder introduces a new paradigm shift in page building and block placement for content entities without introducing a block visibility mechanism.

Proposed resolution

Introduce a block visibility mechanism that includes individual condition configuration on a per block basis.

Remaining tasks

Upload, review and ready a block visibility patch.

User interface changes

Add a "configure visibility" link to to attached actions of a placed block within the LB user interface.

API changes

No changes, just the addition of an optional "visibility" key in the block configuration. Block configuration itself is already separated into a parallel key in preparation for this change, so the existing code anticipates this.

Data model changes

See API changes.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

EclipseGc created an issue. See original summary.

tim.plunkett’s picture

Clearing out the issues pointing at the implementation instead of the plan.

larowlan’s picture

Status: Postponed » Active
tim.plunkett’s picture

EclipseGc’s picture

Status: Active » Needs review
FileSize
27.59 KB

How about something along these lines?

Eclipse

EclipseGc’s picture

Sorry for the trash in that patch. I'll fix it in the morning.

Eclipse

Status: Needs review » Needs work

The last submitted patch, 5: 2916876-5.patch, failed testing. View results

EclipseGc’s picture

Title: Add visibility control to blocks within the layout builder » PP-1 Add visibility control to blocks within the layout builder
FileSize
27.75 KB
1.75 KB

Developed against preview versions of the patches we were dependent on. It's also dependent on #2922033: Use the Layout Builder for EntityViewDisplays, so this won't work w/o that.

Eclipse

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Kristen Pol’s picture

If I would like to test the patch, I'm unclear as to what other patches I need to apply other than from:

https://www.drupal.org/project/drupal/issues/2922033

Not sure what "Developed against preview versions of the patches we were dependent on" means.

tim.plunkett’s picture

I'm not sure, but I believe it's only the last patch from that issue that is needed by this one.

@EclipseGc until that's in, when you post patches can you generate two? One suffixed with -review.patch that is code since the blockers, and one suffixed -do-not-test.patch that contains everything since 8.6.x?

EclipseGc’s picture

Will do. I'll reroll this shortly.

Eclipse

EclipseGc’s picture

FileSize
37.65 KB
17.94 KB

Ok, I didn't have a chance to reroll against only head, so this patch is dependent on #2922033: Use the Layout Builder for EntityViewDisplays comment 62. I will get a version of this patch for core Jan 18th.

There are a bunch of interesting bits in the interdiff. Most importantly, I rewrote how SectionComponents are rendered at all. This will likely need to be split out into a parent issue of this issue, but for the sake of explaining what's here, I converted SectionComponent render array building into an event which is dispatched. This is super powerful because it means other modules can get involved in the render array building process. It also means that layouts COULD potentially render things which are not blocks. Tim wrote the original code that way and I doubled down on it here. I don't think it's a feature we'll practically use, but we essentially get it for free moving to the event dispatcher. Furthermore, being able to run before or after the normal build process means that introducing something like "visibility" into that paradigm became a simple event subscriber instead of me modifying the monolith that did the render work for us (which is what the last patch did).

Net 0 for the SectionComponent class at the moment. The interdiff removes the condition manager that the first patch added, but adds in the event dispatcher service. Still, that's a practical service when I felt that the condition manager really wasn't.

I'll post more tomorrow. (I'm pretty certain visibility condition editing, not creating, is broken. So that's one of tomorrow's tasks.)

Eclipse

EclipseGc’s picture

Also, if you're wanting to try out a full demo of what we're pursuing, this is accurate as of now, and all the patches should layer just fine:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch | git apply; curl https://www.drupal.org/files/issues/2916876-13.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply; composer install

So hopefully that helps with bootstrapping review.

Eclipse

EclipseGc’s picture

Ok, I've rerolled this on top of #2937799: Allow greater flexibility within SectionComponent::toRenderArray() after abstracting that code out of this patch. Also, this should apply directly to core now with only that 1 patch applied. Layout defaults are a separate thing and I don't think it's affected by this code path at all.

I've not provided an interdiff because I actually wasn't certainly how best to get one given that much of the code I removed from the patch still exists in the code base. Whatever the case, the main patch should be significantly smaller now. Fixed a couple of docblocks I'd missed here and there.

Eclipse

EclipseGc’s picture

FileSize
28.98 KB
2.58 KB

Ok, fixed the missing end line I had in the last patch and made editing a configured visibility condition actually work now.

Eclipse

Kristen Pol’s picture

Issue summary: View changes
FileSize
184.85 KB

Hmm... maybe I'm doing something wrong but here's what I did based on comment #14 and patch #16:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply; composer install

And then I added a block but I don't see any visibility info. I don't see it in the contextual links either. Perhaps I'm looking in the wrong place?

tim.plunkett’s picture

Note, contextual links are currently cached *by your browser, per tab*.
After clearing caches, make sure you test it in a new tab.

EclipseGc’s picture

Yeah, it's a totally annoying thing, but you'll need to follow Tim's directions. :-(

Eclipse

EclipseGc’s picture

Also, the patches I've provided are against head and no longer dependent on the defaults issue, so apply these patches, and forgo the defaults patch for the time being (until it lands and I reroll).

curl https://www.drupal.org/files/issues/2937799-8.patch | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply

Kristen Pol’s picture

Thanks for the tips. I see the "Control visibility" in the contextual links now. It works as expected. Here's what I did:

  1. Enabled layout modules
  2. For a basic page node, edited the layout to show the node body
  3. Used the block visibility to only show the body to authenticated users
  4. Verified for admin, the body showed up
  5. Verified for anonymous, the body did NOT show up

  1. I looked at the code as best I could for structure and formatting. Someone else should review for logic. Feedback is out of order in some cases as I went into dreditor a couple times.

    +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +  protected $plugin_id;
    

    Nitpick: Should be $pluginId instead of $plugin_id? (I noticed that was the convention in other places.)

  2. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +    return $this->t("Are you sure you want to delete this visibility condition?");
    

    Nitpick: Use single quotes?

  3. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +    $response->addCommand(new OpenOffCanvasDialogCommand($this->t("Configure Condition"), $new_form));
    

    Nitpick: Use single quotes?

  4. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * Gets the parameters needed for the various url and form invocations.
    

    Nitpick: url => URL?

  5. +++ b/core/modules/layout_builder/src/Section.php
    @@ -68,14 +68,16 @@ public function __construct($layout_id, array $layout_settings = [], array $comp
    +   *   (Optional) Determines if we're rendering a preview or not.
    

    Nitpick: Don't use "we're"... perhaps:

    Whether the component is in preview mode or not.

    which is used in SectionComponentBuildRenderArrayEvent.php

  6. +++ b/core/modules/layout_builder/src/SectionComponent.php
    @@ -91,12 +91,14 @@ public function __construct($uuid, $region, array $configuration = [], array $ad
    +   *   (Optional) Determines if we're rendering a preview or not.
    

    Nitpick: Same as comment above.

  7. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,209 @@
    +   * The block manager.
    +   *
    +   * @var \Drupal\Core\Condition\ConditionManager
    +   */
    +  protected $conditionManager;
    

    Nitpick: Should comment say "The condition manager."?

  8. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,209 @@
    +   * The field delta.
    +   *
    +   * @var int
    +   */
    +  protected $delta;
    

    Question: Field delta? Isn't this the block delta?

  9. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,209 @@
    +   * Constructs a new BlockVisibilityForm.
    

    Nitpick: Constructor above is "Creates a SectionComponentVisibility object." so it would be good to standardize on either that format or this one.

  10. +++ b/core/modules/layout_builder/src/Form/BlockVisibilityForm.php
    @@ -0,0 +1,209 @@
    +    $response->addCommand(new OpenOffCanvasDialogCommand($this->t("Configure Condition"), $new_form));
    

    Nitpick: Use single quotes?

  11. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The plugin form manager.
    +   *
    +   * @var \Drupal\Core\Plugin\PluginFormFactoryInterface
    +   */
    +  protected $pluginFormFactory;
    

    Nitpick: The plugin form factory.

  12. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The field delta.
    

    Question: Same as above for BlockVisibilityForm.

  13. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * The plugin being configured.
    

    Nitpick: The condition plugin being configured?

  14. +++ b/core/modules/layout_builder/src/Form/ConfigureVisibility.php
    @@ -0,0 +1,248 @@
    +   * Constructs a new ConfigureVisibility.
    

    Nitpick: Same as above regarding constructors.

  15. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * The field delta.
    

    Question: Same as above for ConfigureVisibility.

  16. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * The uuid of the condition to be removed.
    +   *
    +   * @var string
    +   */
    +  protected $plugin_id;
    

    "uuid" => "plugin id"

  17. +++ b/core/modules/layout_builder/src/Form/DeleteVisibility.php
    @@ -0,0 +1,162 @@
    +   * Constructs a new DeleteVisibility.
    

    Nitpick: Same as other constructors above.

Kristen Pol’s picture

In addition to #21, I went in to try to delete the condition and wasn't able to get the dropdown to show up consistently. And, the contrast is really bad which I imagine you already know.

EclipseGc’s picture

Yeah, that's a failing of the css reset on the settings tray. An on-going problem in core. :-( I've reached out to people who would know how best to solve it.

Eclipse

tim.plunkett’s picture

Component: layout.module » layout_builder.module