Implementation issue for Layout Builder

You may find this interesting. The recent version of Snippet manager can work as a layout builder (requires Layout discovery to be enabled).

1. +++ b/core/misc/ajax.es6.js
   @@ -269,6 +251,28 @@ function loadAjaxBehavior(base) {
   +    $element.find('.use-ajax').once('ajax').each(function () {
   

   Arrow function.

2. +++ b/core/misc/ajax.es6.js
   @@ -269,6 +251,28 @@ function loadAjaxBehavior(base) {
   +      // For anchor tags, these will go to the target of the anchor rather
   

   Multiline comment format.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +(function ($, Drupal) {
   

   Arrow function, and destructure Drupal.

4. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +
   

   Needs docs.

5. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +    attach: function (context) {
   

   Object property shortcut syntax.

6. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +        update: function (event, ui) {
   

   Object property shortcut syntax.

7. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +          let data = {
   

   Can be `const`

8. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +            region_to: $(this).data('region'),
   

   I'd expect these to not have underscores, but I also understand that PHP is expecting them.

9. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,40 @@
   +          if (this === ui.item.parent()[0]) {
   

   Maybe document why this is needed.

10. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -0,0 +1,40 @@
    +            let url = ui.item.closest('[data-layout-update-url]').data('layout-update-url');
    

    Probably don't need this.

11. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -0,0 +1,40 @@
    +            let ajax = Drupal.ajax({
    ...
    +            ajax.execute();
    

    Don't assign to variable.

12. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -0,0 +1,40 @@
    +              url: url,
    

    Drupal.ajax({url, submit: data}), maybe?

13. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -0,0 +1,40 @@
    +            ajax.execute();
    

    Append .exectue() directly to Drupal.ajax().

@tim Should we create a separate UI issue?

Overall, I'm really happy with this patch. I didn't review the tests yet because I wanted to get the functional parts reviewed first to clear the path for work that could potentially happen at Drupalcon Vienna.

Eclipse

1. +++ b/core/modules/layout_builder/css/layout-builder.css
   @@ -0,0 +1,59 @@
   +  transition: visually-hidden 2s ease-out, height 2s ease-in;
   

   I was reviewing our demo and didn't notice where this bit was applying. Is this applying? how do I see it?

2. +++ b/core/modules/layout_builder/js/layout-builder.js
   @@ -0,0 +1,39 @@
   \ No newline at end of file
   

   twice now I've seen .js files w/o newlines. Is this a thing?

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -0,0 +1,145 @@
   +      $output .= '<p>' . t('Layout Builder provides layout building utility, surprisingly.') . '</p>';
   

   We should probably remove the snark?

4. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -0,0 +1,145 @@
   +function layout_builder_form_entity_view_display_edit_submit(&$form, FormStateInterface $form_state) {
   +  /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display */
   +  $display = $form_state->getFormObject()->getEntity();
   +
   +  $original_value = $display->getThirdPartySetting('layout_builder', 'allow_custom', FALSE);
   +  $allow_custom = $form_state->getValue(['layout', 'allow_custom'], FALSE);
   +
   +  // Only continue if the value has changed.
   +  if ($original_value !== $allow_custom) {
   +    $display->setThirdPartySetting('layout_builder', 'allow_custom', $allow_custom);
   +    $entity_type_id = $display->getTargetEntityTypeId();
   +    $bundle = $display->getTargetBundle();
   +
   +    if ($allow_custom) {
   +      layout_builder_add_layout_section_field($entity_type_id, $bundle);
   +    }
   +    elseif ($field = FieldConfig::loadByName($entity_type_id, $bundle, 'layout_builder__layout')) {
   +      $field->delete();
   +    }
   +  }
   +}
   

   Ok, so this function deletes a field in some cases which probably means we need to confirm that the user wants to proceed with this action when it would result with data loss.

5. +++ b/core/modules/layout_builder/layout_builder.permissions.yml
   @@ -0,0 +1,3 @@
   +configure any layout:
   +  title: 'Configure any layout'
   +  restrict access: true
   

   Seems like we might want to expand our permissions to include per entity (per bundle?) layout configuration and also ownership configuration?

6. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -0,0 +1,122 @@
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   ...
   +    _permission: 'configure any layout'
   

   Per my permissions point, we should probably include a custom access handler here for more nuanced access control.

7. +++ b/core/modules/layout_builder/layout_builder.routing.yml
   @@ -0,0 +1,122 @@
   +    plugin_id: null
   

   I don't recall why we needed to do this. Is this truly needed or left-over cruft?

8. +++ b/core/modules/layout_builder/src/Access/LayoutSectionAccessCheck.php
   @@ -0,0 +1,63 @@
   +      $access = AccessResult::allowedIf($account->hasPermission('configure any layout'));
   

   If we change permissions, we'll need to do something more complex here.

9. +++ b/core/modules/layout_builder/src/Controller/AddSectionController.php
   @@ -0,0 +1,89 @@
   +/**
   + * Returns responses for Layout Builder routes.
   + */
   

   We used to have all the controllers in one class, and I think this docblock might be left over from that time. It doesn't seem accurate to the controller we're looking at.

10. +++ b/core/modules/layout_builder/src/Controller/AddSectionController.php
    @@ -0,0 +1,89 @@
    +class AddSectionController implements ContainerInjectionInterface {
    
    +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
    @@ -0,0 +1,98 @@
    +class ChooseBlockController implements ContainerInjectionInterface {
    
    +++ b/core/modules/layout_builder/src/Controller/ChooseSectionController.php
    @@ -0,0 +1,117 @@
    +class ChooseSectionController implements ContainerInjectionInterface {
    
    +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
    @@ -0,0 +1,306 @@
    +class LayoutBuilderController implements ContainerInjectionInterface {
    
    +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
    @@ -0,0 +1,101 @@
    +class MoveBlockController implements ContainerInjectionInterface {
    

    Is there a reason we're not just extending ControllerBase?

11. +++ b/core/modules/layout_builder/src/Controller/AddSectionController.php
    @@ -0,0 +1,89 @@
    +    $this->classResolver = $class_resolver;
    +    $this->layoutTempstoreRepository = $layout_tempstore_repository;
    

    Super nit-picky, but maybe it'd be nice if these were in the same order as the passed parameters.

12. +++ b/core/modules/layout_builder/src/Controller/AddSectionController.php
    @@ -0,0 +1,89 @@
    +    $field_list = $entity->layout_builder__layout;
    

    So we used to pass the field name to these controllers. When we decided to hard code a field we stopped doing that. I know there's a lot of nuance to that particular problem, but I can't help feeling like it might have been better if we'd hard coded to field name into the route definition's defaults rather than into the code because then contrib routes could potentially "do stuff" w/o being forced to use our field or subclass our controllers... granted the problem is I don't know what "do stuff" means at this moment, so feel free to dismiss this concern. I'm probably just being overly paranoid.

13. +++ b/core/modules/layout_builder/src/Controller/AjaxHelperTrait.php
    @@ -0,0 +1,34 @@
    +namespace Drupal\layout_builder\Controller;
    +
    +use Drupal\Core\EventSubscriber\MainContentViewSubscriber;
    +
    +/**
    + * Provides a helper to determine if the current request is via AJAX.
    + */
    +trait AjaxHelperTrait {
    

    Is this specific to our module? or more broadly useful? Seems like this could be somewhere in Drupal\Core.

14. +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
    @@ -0,0 +1,98 @@
    +/**
    + * Returns responses for Layout Builder routes.
    + */
    

    Same as on AddSectionController.

15. +++ b/core/modules/layout_builder/src/Controller/ChooseSectionController.php
    @@ -0,0 +1,117 @@
    +/**
    + * Returns responses for Layout Builder routes.
    + */
    

    Needs better doc.

16. +++ b/core/modules/layout_builder/src/Controller/ChooseSectionController.php
    @@ -0,0 +1,117 @@
    +      '#title' => $this->t('Basic Layouts'),
    

    This seems like it could be word smithed a bit to be more accurate to what we're doing.

17. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
    @@ -0,0 +1,306 @@
    +/**
    + * Returns responses for Layout Builder routes.
    + */
    

    again, need to update the comment.

18. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
    @@ -0,0 +1,306 @@
    +    $entity_id = $entity->id();
    +    $entity_type_id = $entity->getEntityTypeId();
    

    So after watching the errors that popped up in the demo at Drupalcon, this immediately looked suspect to me. Maybe you've moved something around into a route enhancer or something, but this used to extract the entity from a tempstore if it existed there before trying to do anything here. This code might also be completely ok for reasons I enumerate above, but just saying.

19. +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
    @@ -0,0 +1,101 @@
    +/**
    + * Returns responses for Layout Builder routes.
    + */
    

    docs

20. +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
    @@ -0,0 +1,101 @@
    +    $data = $request->request->all();
    

    We have a lot of $_GET expectations in thsi method, perhaps we should be throwing some sort of exception if we don't get everything we expect?

21. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockForm.php
    @@ -0,0 +1,252 @@
    +    $this->entity = $entity;
    +    $this->delta = $delta;
    +    $this->region = $region;
    

    Should we be throwing exceptions if we don't get all the expected parameters?

22. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
    @@ -0,0 +1,175 @@
    +    $form['layout_settings'] = $this->layout->buildConfigurationForm($form['layout_settings'], $subform_state);
    

    Should we be delegating to the PluginFormManager for this?

23. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
    @@ -0,0 +1,199 @@
    +    if ($access->isAllowed()) {
    +      $block_output = [
    +        '#theme' => 'block',
    +        '#configuration' => $block->getConfiguration(),
    +        '#plugin_id' => $block->getPluginId(),
    +        '#base_plugin_id' => $block->getBaseId(),
    +        '#derivative_plugin_id' => $block->getDerivativeId(),
    +        'content' => $block->build(),
    +      ];
    +      $cacheability->addCacheableDependency($block);
    

    Do we need to be adding the access object as a cacheable dependency just in case dependencies were added to it?

24. +++ b/core/modules/layout_builder/src/LayoutSectionItemInterface.php
    @@ -0,0 +1,16 @@
    +interface LayoutSectionItemInterface extends FieldItemInterface {
    +
    +}
    

    Is this just for type-hinting purposes? I forget the reason we added it.

25. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
    @@ -0,0 +1,98 @@
    +    if (!empty($tempstore['entity'])) {
    

    being full on pedantic here this should probably read:

    if (!empty($tempstore['entity']) && $tempstore['entity'] instanceof EntityInterface) {

26. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldFormatter/LayoutSectionFormatter.php
    @@ -0,0 +1,87 @@
    +  public function viewElements(FieldItemListInterface $items, $langcode) {
    +    $elements = [];
    +
    +    /** @var \Drupal\layout_builder\LayoutSectionItemInterface[] $items */
    +    foreach ($items as $delta => $item) {
    +      $elements[$delta] = $this->builder->buildSection($item->layout, $item->layout_settings, $item->section);
    +    }
    +
    +    return $elements;
    +  }
    

    I love how simple this is despite the complexities of what we're doing. Just saying. :-D

27. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
    @@ -0,0 +1,111 @@
    +          'serialize' => TRUE,
    ...
    +          'serialize' => TRUE,
    

    Ok, so... can we even do this? Or is it ok for experimental code in a pre-alpha state and we just need to convert to json or something before release?

28. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
    @@ -0,0 +1,111 @@
    +    $values['section'] = [];
    

    So when i wrote this I was in a hurry during the sprint or something. I think we should actually generate a block in there. Generating entities that have sample values would mean that we should probably introspect the entity in question and place field blocks (not yet in core) for each of the fields of the entity type in question. So we can totally punt on this comment, I just want to put it out there for the moment.

29. +++ b/core/modules/layout_builder/src/Plugin/Menu/LayoutBuilderLocalTask.php
    @@ -0,0 +1,23 @@
    +    $parameters['entity'] = $route_match->getParameter('entity');
    

    This looks too optimistic to my eyes. Probably something like hasParameter && instanceof EntityInterface.

30. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRouteEnhancer.php
    @@ -0,0 +1,31 @@
    +    $defaults['entity'] = &$defaults[$defaults['entity_type_id']];
    

    So this initially looks too optimistic as well, but looking at how we're generating routes, this could only fail if someone did something really stupid. Is it worth insulating that? Or should we just let it blow up in that case?

31. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutes.php
    @@ -0,0 +1,148 @@
    +    $templates = ['canonical', 'edit_form', 'delete_form'];
    

    So, nitpicky here, but this hard-codes our tasks to only work when on the task itself OR one of these templates. I get the reasoning behind that, but any additional tasks at this level would (if I recall these very old brain cells correctly) not display the "layout" task inline with the others. This is in part due to shortcomings of the task/action menu system, but we'd essentially need to do a lookup against all tasks to get ours to render properly because we have to alter all those routes which is REALLY heavy handed. Again I'm fine with this going in as is, I just want to document the weirdness that's sitting here.

32. +++ b/core/modules/layout_discovery/layout_discovery.layouts.yml
    @@ -56,6 +68,10 @@ layout_threecol_25_50_25:
    +    - [first, second, second, third]
    

    OK, I realize this is a patch we're including for the sake of showing the whole patch working, but this means a 1/3|2/3 icon map for a layout would be [first, second, second]?? TOTALLY weird lol.

1, 2, 10, 11, 16, 18, 21, 22, 25, 30, 32: Great

3: Yup, I think module naming is a topic we should all prioritize at this point. Maybe next scotch meeting.

4: Yeah, I recognize this is a unique-ish issue.

5 & co: Fair enough. The todo actually satisfies me here if committers are fine with it, I just wanted to call out that we needed more nuance in this area.

7: I was just thinking the controller could do an = NULL in the method, but I'm fine with this solution too.

9, 14, 15, 17, 19: todos++

12: Ok, if we're both paranoid about this, that makes me feel better. :-D

20: The MoveController specifically is called via some JS, so I think I was just working with what it naturally did. I have no problem with cleaning this up in the way you suggest, just justifying why it is how it is.

23: How did I miss that?

27: I did write about half of that and I think you copied it for the other half (layout configuration). Point being yeah... I think for security purposes we need to be doing json here instead of PHP serialization since this is field values on an entity. It's unfortunate that the serialize blog stuff doesn't just support a format so that we don't have to manually intervene to make json happen.

28: Sorry, yes that's what I was referring to. And also yes, let's do a follow up for it. We need to get a few other things committed before we could even entertain what I'm asking for, so definitely follow up material.

29: Yeah things are a mess. Subscribed.

31: Ok, I need to apply this patch and play with that. If you could remove all this with that one change I'm SUPER ++.

Again, I want to reiterate, I'm super happy with where this patch is going, so I think we just need to enumerate which of the remaining issues we are going to address for our MVP before commit. Things like 27 could probably be handled in a follow up if we're ok with HEAD having a known security issue in an experimental module. I just don't know what the standard we're attempting to achieve here is and I think that's my main ask at this point.

Awesome stuff!

Eclipse

Saw this in DrupalCon Vienna 2017 and gave it a try and here is my findings:
It's so nice to see both in-place editing and system tray work very well with layouts. It also seamlessly adds panes/blocks without page reload. Here is few minor UI issues(IMO):
1. The drag-and-drop option is hidden as the cursor doesn't change (stays as pointer)
2. Position of add block link (refer attached screenshot)
3. Not sure about the purpose and design of save and cancel layout links.

1.) Dyannenova has been iterating on the css. We're aware of and agree that this (as well as drop placement indication) are problems.
2.) Yeah, I've not looked into what's going on with that yet, but it is annoying. Probably a css issue as well.
3.) So this one actually deserves some justification. If we've not done that elsewhere, I'll do it here.

Collectively, we'd like to use the administrative toolbar for our buttons (example screenshots are here #2884601: Add a Layout Builder to core), however the code for doing that was UGLY. We punted on this as the toolbar needs a lot of love right now, and that is certainly a separate effort from our own. Understanding that, we needed a different place for our save/cancel buttons. It's been suggested multiple times that we should put them in the settings tray forms, however those forms change regularly (block placement, layout placement, etc) and worse still, the settings tray can be closed. In order to satisfy our need, we placed the save/cancel links as sub-tabs of the layout tab because it will always be available to the user. True, they're not actual "buttons" but the links suffice just fine for our needs and are always available to the user while working on these screens.

Ultimately, we'd like to migrate this back to the toolbar per the screenshots I referenced earlier, but until such time as the toolbar can support what we need, this seemed the best compromise.

Eclipse

Thanks for your time to address those items and good to hear from you @EclipseGc as always.
1. Great! thanks for the details
2. On someone's list. That's good :)
3. I am not a UI person, not going to suggest anything other than just point out, admin toolbar could get crowded once we start moving more items (like Workspace) there. Probably we have to have some rules/standards around what can be part of admin toolbar. Should it be side wide or page level etc. Another way of looking at it is to find a common UI for things like this. I don't know, if views UI or block placement UI fall under this category, but either a) we take one of those existing pages as common b) we come up with a new UI that works for all?

awesome! I was not looking forward to that. :-D

Adding myself to commit credit since I helped write a lot of this code.

Eclipse

i wouldn't mind taking myself out of commit credit as it was just a single review(with a screenshot file). Considering the amount of work done by @tim.plunkett and @EclipseGc, it's negligible.

We reviewed this at today's product management meeting. You can see the recording here: https://www.youtube.com/watch?v=fAGQToEOgTQ

No blockers for committing this patch as an experimental module from our team's POV. The patch looks and works great. It's a very exciting, significant user-facing change, and a huge "selling point" for 8.5.

For stable:

- We need to work a bit on the "save/cancel" button placement. They're currently in secondary tabs, which we know are hard to see, and they're not styled like buttons, which compounds the issue. (Emilie has mocks for moving these to the toolbar, needs a sub-issue for design exploration... the issue about the general problem with toolbar is at #2877853: Make toolbar items first class menu citizens) (must-have)
- Dries pointed out that adding Yet Another Tab™ here is an anti-pattern as well. Better would be to combine this with edit mode, else it's confusing how "edit" only edits content, yet "layout" (which doesn't mention editing) is for editing layout. Needs a follow-up for design exploration. (must-have)
- Because the list of blocks in the sidebar is just a set of links, there's going to be a lot of "pogo-sticking" with this UI: clicking a link to add the block, seeing how it looks, realizing it's not the right one, deleting it, repeating. Would be great if there were some means of getting a visual representation of the block you're about to place before hand. Needs a sub-issue for design exploration. (should-have, since it doesn't make the existing situation worse, though it's very important given this is a more user-facing screen vs. an admin-facing screen.)

The accessibility team should also review this before committing to get their take on commit-blockers vs. stable-blockers. We noted some things like there's no selection in the sidebar for weight / region. Not sure if tabbing works throughout, etc. Tagging "needs accessibility review."

The question of upgrade paths also came up, it sounds like the Panelizer team is committing to writing an upgrade path between that module and core, so the official recommendation should be "build with Panelizer."

AMAZING work, all! We are SO excited for this!! :D

So first, can I just say "YAAY!". That conversation was enlightening in a bunch of ways and was really good to hear. I'm glad the product manager review went this direction.

To speak to some of the issue brought up here (and add additional color).

36.1 & 36.2) Agreed all around. I would love to get these buttons back into the toolbar. It feels like the right place for this, and building an expandable pattern of administrative tools there would be really great in my opinion. That being said, Wim's comment on #2877853: Make toolbar items first class menu citizens is sort of disheartening (and he's totally correct) so I worry that that effort might be greater than this one has been. Bojhan mentioned a "3rd option" that never seemed to be expounded upon. I'd love to hear what he's thinking.

36.3) Also agreed, having block previews would be awesome. panopoly_magic module did this in D7. There have been a number of discussions on this topic over the last few years now as it pertains to 8. This is a pretty nuanced topic and will likely require its own efforts at some later date. My biggest worry here is that the UI will likely need some significant love to make it visually grok-able and not a cluttered mess.

36.4) Accessibility has been a concern since the beginning, but we've worked pretty fast and furious through this whole effort and haven't had the time to really allot to it. FWIW I was envisioning a separate "move" contextual action that would be visually hidden. That UI would parse the layout field for deltas, regions and block placement and represent that data in a simple form with select lists. As Tim mentioned, "weight" isn't even a concept in this UI because blocks are placed within the array at the appropriate point for rendering. Weight was introduced to Block entities for use within core's normal block rendering pipeline, but these are separate things (despite the overlap of the word "block" in both places hehe). I'll of course wait for the accessibility review, but I wanted to at least document what had been in my head in terms of solving this problem (or initial steps toward solving it).

36.5) I'll also point out that the Lightning team is also incentivized from an "upgrade" front. Panelizer makes the most sense in terms of where the effort should happen. I didn't realize Jakob was that degree of committed to writing the upgrade path there but cool.

Thanks so much again for this review. And again "YAAY!"

Eclipse

I believe the "third option" that @Bojhan alluded to is hinging it off the "Preview bar" pattern:

@Bojhan was going to open an issue tomorrow though that deals less with the specific solution but more around the general problem space of "where to stick these buttons." But this approach theoretically seems like it would nicely avoid the implementation challenges around Toolbar.

EDIT: Looks like this is what panopoly_magic does: a pop-up screen that previews the block and allows you to set settings prior to doing the placement. Interesting.

And right, "weight" in this context is a short-hand for "make sure there's a way to reorder blocks within a section and a way to move blocks up/down to different sections that doesn't involve a mouse." :) @EclipseGc and I were discussing at BADCamp and seems like this would be useful for mobile as well. Drag and drop is often a disaster when you can't see the full page. ;)

panopoly_magic does what webchick is showing there (you can change the block settings on the left, and they are reflected immediately in the preview on the right via AJAX). But, also, on the screen before that screen, where you're picking which block to add, it shows the blocks rendered so you can see what it looks like before even selecting which block you want.

We've gone through a couple iterations of both those "screens" based on users' experience with them over the years, and a couple of them are still available via configuration options (because we didn't want to change existing sites too much as stuff evolved), in case anyone is curious.

Adding issue credits.

Adding the requested followup:

https://www.drupal.org/node/2917777

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutSectionTest.php
@@ -0,0 +1,351 @@
+  public function providerTestLayoutSectionFormatter() {

So, I recently proposed that a different issue use a data provider for a BTB test, and got pushback because it apparently adds multiple minutes of test run time to install Drupal over and over. I wanted to overrule and say it was worth it for maintainability (the alternative being the mother-of-all-nested-foreach-loops), but OTOH slow test run time is also a contribution barrier, so we should at least consider this before committing a data provider with this many dimensions.

Patch looks great overall.

Code review, no manual test yet.
This looks really good, great job.
The follow-ups look good.

Re @xjm's question about the setup cost of a data provider, I think its worth it for the long-term maintenance sake.

Code review observations.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -0,0 +1,37 @@
   +      $(context).find('.layout-builder--layout__region').sortable({
   

   do we need a .once here?

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -0,0 +1,147 @@
   +  array_unshift($form['actions']['submit']['#submit'], 'layout_builder_form_entity_view_display_edit_submit');
   

   Should this use an #entity_builder instead? That would allow it to 'run before the entity is saved by the form'

3. +++ b/core/modules/layout_builder/src/Controller/AddSectionController.php
   @@ -0,0 +1,91 @@
   +      $url = Url::fromRoute("entity.{$entity->getEntityTypeId()}.layout", [$entity->getEntityTypeId() => $entity->id()]);
   

   If we altered the entity to add a 'layout' link template, we could use $entity->toUrl('layout') here. Would be more convenient elsewhere too.

4. +++ b/core/modules/layout_builder/src/Controller/AjaxHelperTrait.php
   @@ -0,0 +1,38 @@
   +      'drupal_ajax',
   +      'drupal_dialog',
   +      'drupal_dialog.off_canvas',
   +      'drupal_modal',
   

   There are more wrapper formats in contrib and client projects than listed here. Perhaps in a follow up this could be a container parameter.

5. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -0,0 +1,309 @@
   +            'attributes' => [
   +              'class' => ['use-ajax'],
   +              'data-dialog-type' => 'dialog',
   +              'data-dialog-renderer' => 'off_canvas',
   +            ],
   ...
   +        '#attributes' => [
   +          'class' => ['use-ajax', 'configure-section'],
   +          'data-dialog-type' => 'dialog',
   +          'data-dialog-renderer' => 'off_canvas',
   ...
   +        '#attributes' => [
   +          'class' => ['use-ajax', 'remove-section'],
   +          'data-dialog-type' => 'dialog',
   +          'data-dialog-renderer' => 'off_canvas',
   

   there are a few instance of this pattern - worth a helper method? buildAjaxLinkAttributes($additionalClass = [])

6. +++ b/core/modules/layout_builder/src/Controller/LayoutRebuildTrait.php
   @@ -0,0 +1,58 @@
   +    $layout_controller = $this->classResolver->getInstanceFromDefinition(LayoutBuilderController::class);
   +    $layout = $layout_controller->layout($entity);
   

   nice

7. +++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
   @@ -0,0 +1,107 @@
   +      if (empty($values[$region_to])) {
   +        $values[$region_to] = [];
   +      }
   +      $values[$region_to] = array_merge([$block_uuid => $configuration], $values[$region_to]);
   

   minor: could do $values = $values + [$region_to => []]; and save some lines?

8. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockForm.php
   @@ -0,0 +1,254 @@
   +      $plugin_id = $field->section[$region][$uuid]['block']['id'];
   +      $configuration = $field->section[$region][$uuid]['block'];
   

   Are there instances where these keys may not exist? Should we be more defensive?

9. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockForm.php
   @@ -0,0 +1,254 @@
   +    $form_state->set('block_theme', $this->config('system.theme')->get('default'));
   

   bugger

10. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
    @@ -0,0 +1,213 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function buildForm(array $form, FormStateInterface $form_state, EntityInterface $entity = NULL, $delta = NULL, $plugin_id = NULL) {
    

    We should document the additional parameters here - especially the instances in which they can be NULL. Took me a while to understand that plugin_id is only passed for new items, at first I thought the $this->isUpdate = is_null($plugin_id) was back-to-front.

11. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
    @@ -0,0 +1,213 @@
    +    $this->delta = $delta;
    ...
    +      $field = $field_list->get($this->delta);
    ...
    +      $field_list->addItem($this->delta, [
    

    $delta can be NULL according to signature - should we validate that? I realize it has to have the = NULL to satisfy the interface, but should we enforce it isn't NULL in the body?

12. +++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
    @@ -0,0 +1,213 @@
    +      $field = $this->entity->layout_builder__layout->get($this->delta);
    

    Same for the entity?

13. +++ b/core/modules/layout_builder/src/LayoutSectionBuilder.php
    @@ -0,0 +1,199 @@
    +    $cacheability->applyTo($result);
    

    offtopic: we should open an issue to have applyTo return the render array as well, so we could return $cacheability->applyTo($result);

14. +++ b/core/modules/layout_builder/src/LayoutSectionItemInterface.php
    @@ -0,0 +1,16 @@
    +interface LayoutSectionItemInterface extends FieldItemInterface {
    

    Should this be marked @internal?

15. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
    @@ -0,0 +1,102 @@
    +class LayoutTempstoreRepository implements LayoutTempstoreRepositoryInterface {
    
    +++ b/core/modules/layout_builder/src/LayoutTempstoreRepositoryInterface.php
    @@ -0,0 +1,60 @@
    +interface LayoutTempstoreRepositoryInterface {
    

    Should these be internal too?

16. +++ b/core/modules/layout_builder/src/LayoutTempstoreRepository.php
    @@ -0,0 +1,102 @@
    +   *   An array containing the collection name and the tempstore ID.
    ...
    +  protected function generateTempstoreId(EntityInterface $entity) {
    ...
    +    return [$collection, $id];
    

    I am not a fan of this pattern, but a value object seems like overkill.

    However, if we had a method getTempstoreCollection and kept the generation of the collection name internal to that, we could do away with this and have it solely return the ID. Outside of the exception, we don't use the collection other than to get the tempstore. Does it add that much value to the Exception? We could just use the entity type ID.

17. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldFormatter/LayoutSectionFormatter.php
    @@ -0,0 +1,87 @@
    +      $elements[$delta] = $this->builder->buildSection($item->layout, $item->layout_settings, $item->section);
    

    nice and clean <3

18. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutes.php
    @@ -0,0 +1,124 @@
    +      $route = (new Route("$template/layout"))
    

    As mentioned above, I think we should add an entity type alter hook to add a link template for this too?

19. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutes.php
    @@ -0,0 +1,124 @@
    +          $entity_type_id => '\d+',
    

    Not all content entities have numeric IDs. This is done conditionally in other places in core, see the entity route builder handlers.

+++ b/core/modules/layout_builder/src/Controller/MoveBlockController.php
@@ -0,0 +1,107 @@
+    $configuration = $values[$region_from][$block_uuid];
+    unset($values[$region_from][$block_uuid]);
+    $field->section = array_filter($values);
+
+    /** @var \Drupal\layout_builder\LayoutSectionItemInterface $field */
+    $field = $entity->layout_builder__layout->get($delta_to);
+    $values = $field->section;
+
+    if (isset($preceding_block_uuid)) {
+      if (!isset($values[$region_to])) {
+        throw new \InvalidArgumentException('Invalid region');
+      }
+
+      $slice_id = array_search($preceding_block_uuid, array_keys($values[$region_to]));
+      if ($slice_id === FALSE) {
+        throw new \InvalidArgumentException('Invalid preceeding block UUID');
+      }
+
+      $before = array_slice($values[$region_to], 0, $slice_id + 1);
+      $after = array_slice($values[$region_to], $slice_id + 1);
+      $values[$region_to] = array_merge($before, [$block_uuid => $configuration], $after);
+    }
+    else {
+      if (empty($values[$region_to])) {
+        $values[$region_to] = [];
+      }
+      $values[$region_to] = array_merge([$block_uuid => $configuration], $values[$region_to]);

One final thing, one of the consequences of Drupal's consistent use of arrays is we end up with lots of scattered logic. Having had to manually update a lot of panelizer config in an update hook before, I've found dealing with these big structured arrays difficult in the past.

Given this is just getting serialized, we could introduce a domain object here. And move all of the logic that is currently in controllers into that domain object.

e.g.
$section->moveBlock()

$section->removeBlock()

$section->insertBlock()

At the present all of this logic is in the controller, and if someone finds themself having to update a lot of them in bulk (e.g. in an update hook), we have to drop back to replicating this logic.

Yes, I realise this is how panelizer has done it since D7.

But I think it would be worth the pain.

The net result would be the controllers would be far thinner and we'd have domain logic we could test without a functional test.

Yeah, this is looking really nice

@tim.plunkett++

I just watched last week review (https://www.youtube.com/watch?v=fAGQToEOgTQ). It's all looking pretty damn smooth already and the main points for followups have been identified. I'm in favor of committing this sooner rather than later.

To get to Stable status it would be nice if we tested this with non-core developers as well. You know, to validate that we've built the right thing and for identifying where to focus further refinements and/or extend the capabilities.

Super great work team!

I don't have a clear enough sense yet of what is blocking vs. followup for this because I wasn't able to test the patch fully (something is broken). I checked and the summary of #2884601: Add a Layout Builder to core doesn't have a list yet of what's scoped to followup issues vs. remaining tasks for this patch, nor does this summary. I think it would be good to start enumerating things and sorting them between those two categories (plus listing things that haven't been sorted one way or another yet), to avoid a repeat of the Great Outside In Issue Comment Review Struggle of 2016 and to avoid reviewers like me repeating feedback that's already been given or scoped to followups. I'd propose something like:

Remaining tasks

Blockers

• #49.3: Pretend larowlan said something blocking here' that's not addressed
• #61.4: Figure out why xjm gets no layouts. Regression?

Followup roadmap (to be prioritized later)

• #56: Validate design with non-core developers
• #2917777: Improvements to the styling, grouping, etc. of the Layout Builder UI actions form
• #63.1-3, 7: Figure out a cleaner foo with the whatsit

Unaddressed feedback

• #61 (all points)

Wontfix

• #47 (test run time was 50 min on https://dispatcher.drupalci.org/job/drupal_patches/35694/, not noticeably different from the mean test run time)

And so, miscellaneous things from testing.

1. I think there might be some missing cache invalidation or something. First of all, it took me forever to find the "Layout options" fieldset on /admin/structure/types/manage/article/display. But even after I did, and checked and enabled it, the layout tab didn't show up and so I could not for the life of me figure out how to use the patch. I would not have been able to get anywhere without @webchick's video. :) The tab appeared after a cache clear.
2. I assume that the long-term plan will be to convert both the field UI and the block library to the same interaction pattern; is that correct? Because having three different interaction patterns for laying out the page is very confusing and overwhelming.
3. I think the content layout needs to default to a single layout with the main content area. Otherwise, someone could find and check the custom layout box like I did, never discover the layout tab, and never understand why content they input into Drupal never shows up. Seems like a serious UX issue.
4. I, um. I also set up a whole layout (with an impressive array of breadcrumb, menu, help, and search blocks) but it never seems to be applied after I save the layout. The "View" tab remains forever empty, no matter how many times I log in/out, clear the cache, etc.
5. Does a layout in progress go into a tempstore like Views? Seems like the kind of thing where it would really suck to lose your work when leaving the page. I couldn't tell whether it did or not when playing with it.
6. On that note, would be good to have a message like "You have unsaved changes" like Views does. Otherwise I can get lost in the page and not know if I actually changed something or not.
7. IANAD (I am not a designer) but it seems like it would be good to have visual feedback on when a layout is saved or not.
8. It's also kind of hard to see the difference between sections and regions. The gray vs. blue dotted borders are very subtle. Do they pass a contrast ratio with each other?

Can't really review more until it works. :P

Oh I had a couple more things:

1. Can't read what I'm confirming:
   
2. This point removed; feedback was duplicated across two issues.

On #61.4, I had hacked up the patch to test with the latest icon generator, and using the actual patch from #58 did actually make the layouts show up. So maybe I just made a mistake that introduced the regression. Assume that point can be disregarded for now. No this is actually broken; see below.

Another thing that I found confusing. Removing a whole section has a visible x button, but removing a block from the layout requires using the contextual links that you can only see when hovering over the block, and there's very little feedback as you move the cursor over the page as to where the block is and isn't. Because there aren't any boundaries around the block separating it from the rest of the region, which includes both other blocks in the region and whitespace.

I wonder if the Settings Tray hover interaction would be a good model?

Oh! It wasn't because of me hacking up the patch after all. If you have Field Layout enabled, the Layout Builder's layouts are ignored and the content appears empty forever (not even displaying the main content). That seems like a critical issue to me.

It was also hard to tell when a section is added; at first, I didn't understand what was happening with the sections. The sections end up sort of below the fold, and when you have multiple sections, there's no visual feedback as to where the new one goes. It seems like a final version, maybe not the MVP, would do the thing where the new section is highlighted when it appears and then fades in to the normal colors.

xjm

61.0: Yeah I'll see if I can devote some time tomorrow or Monday to fixing up the IS on this issue. Generally speaking the only thing we consider a blocker currently is #2660124: Dynamically build layout icons based on well formed config. Obviously pending the various reviews we have that could change, but as far as external blockers, that's the only one we are blocked on.

61.1: Confirmed, I've seen this too.

61.2: Field UI for certain. That's on our immediate todo list after this is committed. So much so, we'd favor pulling this code out of 8.5 if we can't convert the field UI to this mechanism as well. We don't want a generation of Drupal sites that use this code w/o sane default options for content layout. That would be a nightmare.

With regard to your question about core's Block Layout, I've been turning some of my attention toward that problem mentally, but we have not discussed it as a group at all. Having Entity view modes and individual entities themselves properly laid out with this tool is a complete enough state that I think we are all exclusively focussed on that idea for the time being. It's functionally "panelizer in core" which hits the 80% use case for certain. Don't get me wrong, I'd like to do "all the things" but there are no plans of any sort yet in place for such a future.

61.3: I'm not sure I follow what you were trying to say in this point. Maybe elaborate a bit more? It sounds like you were discussing having some sort of sane default? If not I apologize for the following response.

Our next goal is to get entity view_modes working with layout. Part of that would be to copy in the full view_mode's layout into the entity specific layouts as a "starting point" for doing a custom override. That way, if there was just some small change to be made, it'd not require the content author/site builder to rebuild the default layout from scratch for each override. Again, if this was tangential to what you were saying, I apologize. Just try again and hopefully I get it this time :-D

61.4: Weird, I've never had that problem... do you have field_layout installed at the same time? Might be the modules are stepping on each others toes? I've also not tried the newest new patch, but tests should be failing if this were the case. I'll apply it and try it this weekend.

61.5: yes, we use tempstore extensively

61.6: Fair point. Do you feel that needs to be addressed now? or would a follow up issue be sufficient?

61.7: So again, fair point. Counter point to that is that when you save it you're immediately dropped back onto the entity's canonical link, so I guess I felt that communicated "hey it's saved" pretty clearly since you were seeing it in action after you hit the save button. That said, I guess I'd prefer to let DyanneNova address whether or not that's a concern. I'm just expressing my experience with the system (which again... since I coded a fair bit of it, my expectations could be TOTALLY wrong cause I know what happens when you click that link hehe).

61.8: 100% agree. I think we all do. FWIW I think we're all contemplating something more like this for the next big iteration of css changes:

62: Yeah, we're aware. I question whether or not this isn't an issue for the settings tray elements themselves (as opposed to layout builder). Sort of feels like we're just exposing an existing shortfall of that toolset. That being said, we could certain consider using modals in this case. I kind of like having it in the sidebar though. Again, I defer to DyanneNova on this.

64: So, in answer to this, I think I'd point back to the screenshot I included for 61.8. I think we'd like to have a bit of administrative UI attached to the regions (and I'd like to do similar with the blocks). Kind of unify the "stuff you can do" at each of these levels while bringing some visual clarity to the UI at the same time. As usual, I defer to DyanneNova on this, I'm just documenting some of what we've passed around in back-channels on this topic. Again I'd ask, is this in "follow up" territory in your opinion? or should we be addressing it in this patch?

65: Is this what you were saying in 61.4? I read all your comments before responding, so I assume that's where I got the idea. :-D

66: Yeah, again referring to 61.8 here but I like the idea of maybe scrolling to the newly added section and temporarily highlighting it somehow. I think it'll be less confusing once the sections and the "add section" elements don't have the same visual authority on the screen, but I'm open to anything that helps communicate to the user how to use the tool, and what effects their actions are having.

Again, I'll try to get to the IS tomorrow or Monday. Thanks for the review, this is all good stuff. Sorry it and field_layout don't like each other yet. I don't think any of us have tested with both of them on (not that I know of). We should put a disclaimer somewhere for the time being. heh.

Eclipse

Just a list of a few accessibility issues as part of a quick review:

1) the "Choose a layout" panel that popus up after clicking on "Add Section" has no focus or hover states. It is very difficult to find out where you are and how to close the window. Some simple CSS should fix this.

2) Any time content is being changed via JavaScript, we need to alert screen reader users that the page is no longer the same. Adding the layout should come with a call to Drupal.announce() so that a blind user is alerted in a semantic fashion.

3) After adding a new layout, the focus should go to the new layout that has been added rather than simply landing at the top of the page. The first thing a person should tab to after adding a new layout is the new layout they have added.

4) When adding a block to a layout there is the same problem with lack of focus/hover that is mentioned in #1. It actually might be there but it is very faint and there is a bit of a delay so hard to notice.

5) As Tim pointed out in slack, there are some color contrast issues. #46a0f5 on #222222 doesn't pass.

6) As with #2 Any time content is being changed via JavaScript, we need to alert screen reader needs to be alerted with Drupal.announce() so that a blind user is alerted to the new content.

I'm happy to have a look later on this.

In Slack, Tim let me know that some of the issues in #73 probably should be dealt with in other components.

An area I didn't discuss in much detail was the drag/drop functionality. I wanted to remind folks that this is how we dealt with it in Drupal 7 because we didn't have access to a stable version of WAI-ARIA - #448292: Drag and Drop for table rows is not accessible to screen-reader users

Some possible code to consider

• https://www.w3.org/blog/wai-components-gallery/widget/accessible-drag-an...
• https://tink.uk/jaws-11-aria-drag-drop-support/
• https://codepen.io/barrytsmith/pen/kfiqj
• http://lisafolkerson.com/building-an-accessible-drag-drop-user-interface...
• https://dequeuniversity.com/assets/html/jquery-summit/html5/slides/dragd...

Re: #73

5) As Tim pointed out in slack, there are some color contrast issues. #46a0f5 on #222222 doesn't pass.

This colour combination has a contrast ration of 5.8:1, so it does pass... but which element are you talking about? I can't find #222222 anywhere.

After adding a section or block in the settings tray, focus is lost (it returns to the start of the document). It would be better if it returned to the same "add section" or "add block" button that the user operated to reach the settings tray.

At the moment you have to be sighted to get an understanding of the layout. There's no human-readable content in the div#layout-builder which describes the layout; all the layout information is in class, data-*, and href attributes, which are really only developer-readable.

We'll have to find a human-readable way to describe the layout (and your current position inside it...) to assistive tech users. This isn't going to be easy! Two obvious semantic structures spring to mind:

• HTML tables. Screen readers and other assistive tech can already understand these, and many screen readers have advanced tools for interrogating the structure. For instance a user can press key-combos to announce the current cell coordinates, or speak the row/column headers.
  Pros: in theory we get a lot for free, between the HTML semantics and screen reader tools.
  Cons: the grids here are likely to be quite complicated and irregular, so even using the screen reader tools, a user may easily become lost. There's a likelihood we would end up using nested tables. There's also a slight semantic mismatch; tables are containers for content; the cells aren't usually expected to move around!
• An ARIA grid pattern, such as the ones described in the WAI-ARIA Authoring Practices working draft. Instead of HTML semantics, ARIA roles and properties provide the semantics (role, aria-labelledby, aria-describedby, and possible the new aria-roledescription).
  Pros: they're not tables, so there are fewer assumptions about how they work. We may achieve it with simpler markup, as there's less expectation of a uniform grid.
  Cons: We get less for free, we have to define all semantics carefully. ARIA 1.0 core is a full rec, but the authoring practices is still a working draft.

Whichever approach we take, we'll likely need a lot of visually hidden text, and manage IDREFs for aria-labeledby & aria-describedby, and some custom drupal.announce() messages too.

I have no idea how to build this. I've not inspected many layout builders like this in the wild, let alone found an accessible one. AFAIK there isn't yet a tried and tested best practice for this, a11y-wise. The ARIA grid pattern only gets us so far. It doesn't cover parts like telling a user what has changed as a result of their actions.

We can ask around the a11y community to see if anyone has built something like this.

The closest thing to this that we've built is the CKEditor toolbar configuration UI. Note that doesn't use ARIA grid, but it does have a lot of custom announcements for screen readers. We also have had some feedback from screen reader users who are confused by it (#2349309: Improve CKEditor toolbar configuration usability).

Patch in #75:

+      $build[$category]['links'] = [
+        '#type' => 'table',
+      ];

We're generating tables with just a single cell per row. Wouldn't a UL be simpler?

Via @webaxe on Twitter, we've learned of DragonDrop (demo) which has a nice(-ish) keyboard and screen reader experience.

Here's an initial review of the DragonDrop demo:

• It only has examples of draggable rows, not 2-dimensional movement.
• The first example has clear grab-buttons with a clear icon, text fallback, and focus styles.
• The grab behaviour isn't as discoverable in the second example.
• The grab-button is a <button> so we'd expect enter and spacebar to both activate it. However enter and spacebar have two different behaviours:
  • Enter toggles the grab state. To move it you either press enter (grab), arrows keys, enter (ungrab).
  • Spacebar acts more like a shift key, so it's grabbed while spacebar is held down. To move it, press spacebar + arrow keys together.

  This different behaviour is a bit hard to discover. I initially pressed + released spacebar, then used arrows. My first impression was that it was broken.

• Escape cancels the re-ordering, and leaves things where they were before you tried dragging. Nice, assuming you discover it.
• aria-live announcements say what the new position is.
• The DragonDrop has it's own aria-live region, i.e. we wouldn't use drupal.announce(). This is fine, in fact it's already the case that our autocomplete widgets have their own aria-live region.
• The aria-live messages tend to queue up, so you can sometimes hear a long sequence of grabbed/ungrabbed messages. Meh, needs a throttle.
• The grab-buttons are all aria-describedby a piece of visible help text. This gets announced after every movement, which is very verbose and repetitive. It will likely respect the verbosity settings of different screen readers, but they don't all offer this. I didn't like this part.
• The DragonDrop demo does not use HTML tables or ARIA grid semantics. It's just draggable rows, so it gets away with an ordered list. For our layout builder, I still think we'll need ARIA grid.

I think we should start a separate issue to explore accessible drag & drop grid patterns.

Added #2920006: Research accessibility of drag-and-drop grid interfaces.. I'll copy most of my previous comment over to there, as the first library reviewed.

So um. The latest patches seem broken. When I click "Add section" something burps and the sidebar does not open.

drag Layout to enable -> click Save -> Error
https://dur4v.ply.st/admin/structure/types/manage/article/form-display

I think this issue focus on CODE (main features) more than UX/UI. But I also drop my comments here. It's an MVP and some issues might be discussed already and I think the contributors able to pick my suggestions to right channels.

Add Section
- Provide a way to add single row column rather than a whole layout. eg. 2 column row (better UX actually)

Add Block
- Core: Page title
If you add it without "Display title", it's nothing. It's a bug to a regular user. (bad UX)

- Core: Tabs
Tabs has no visible UI

- Forms: User login
It needs an API to hide these empty blocks I think.

- CSS BEM naming issues

====================================================================================

I suggest to change the way to add a new block:

I'm a long time WordPress user also. They have many 3rd party layout plugins already. It's a very traditional way to add things (Add Section -> Select Block). It's horrible and painful I can tell from my experiences.

My idea is similar to https://snack.expo.io/

LEFT SIDE:
Design the Layout

1. Click "Add Section"
2. Show a menu (popover/megamenu) below the "Add Section" button
3. Select a Layout
4. Hide menu
5. Insert new added Layout.

RIGHT SIDE:
1. A fixed panel on the landing page:
- 1.1: With Floating / Pin feature (WHY? Do not affect the display area WIDTH, no RWD)
- 1.2: Able to collapse into a small icon
- 1.3: A checkbox for turn on/off default config for all dragged block
- 1.4: With keywords filters

2. Loaded a list of block on the Panel
3. Drag a block from list to LEFT section
- 3.1: If we checked 1.3 to default config, then that's all.
- 3.2: If we unchecked 1.3, then show a Detail Config Panel

Why is this better?
`For end users, we always want to PREVIEW the result!!!`

This way we knowing what blocks we have on hand first. We design the layout on the left quickly and then drag the block into different sections (This is the 2nd time to design our Layout). Usually, it's like a prototype process more than one-step to final. For most users, they don't know how it works and can't imagine the whole pictures of final results.

It even able to introduce a CONFIG MODE for batch config editing. (Saving few more clicks)

I think we're basically at our MVP barring review from Framework and Release Managers to the contrary. Marking RTBC.

Eclipse

tagging for sprint next week

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -0,0 +1,200 @@
   +  unset($form['fields']['layout_builder__layout']);
   +  $key = array_search('layout_builder__layout', $form['#fields']);
   +  if ($key !== FALSE) {
   +    unset($form['#fields'][$key]);
   +  }
   ...
   +  _layout_builder_hide_layout_field($form);
   

   If only we had a ->setDisplayConfigurable for config fields

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -0,0 +1,200 @@
   +  if ($original_value != $new_value) {
   

   nit this could be !==?

3. +++ b/core/modules/layout_builder/src/Access/LayoutSectionAccessCheck.php
   @@ -0,0 +1,68 @@
   +      $access = AccessResult::allowedIf($account->hasPermission('configure any layout'));
   

   Should this use ::allowedIfHasPermission() so that the permissions cacheability metadata is added?

I'm happy with the architecture, but as discussed with Tim via irc/slack - would be keen to see what the data model will look like for #2922033: Use the Layout Builder for EntityViewDisplays

moving back to rtbc

Eclipse

It's great to see this RTBC.

@catch and I agreed that release managers don't need to totally sign off on adding this as alpha now that we only ship beta+ modules, unless we have concerns with the maintainability of adding them at all. Layout Builder obviously belongs in core so we're comfortable with this going in as alpha. Untagging for release manager review for that reason.

The release manager will want to do a thorough review before the module is tagged as beta (and therefore kept in a release). That also means we might make updates to the roadmaps based on our reviews.

Tim and I discussed a couple things in person yesterday:

1. I think the task to make this same interaction for laying out the templates for an entity display (the replacement for field layout) needs to be a must-have on the roadmap. If it's already there, please retitle it to be clearer; if it isn't, please add it because Tim said he agreed it was a must-have. :)
    
2. Emilie's designs show content in the blocks when you place them. This patch doesn't do that. I don't think it's required for a stable version unless the product team says so, but it's definitely the thing that makes the designs look "wow" and that will make the feature more intuitive for people. Can we make sure there's a roadmap issue specifically for that, or title the existing issue that will do that more clearly?
    
3. Similarly, there's some design issues with the borders of the section and the padding that I know Tim scoped to a followup issue but I don't see it in the summary yet.
    
4. #91 sounds really interesting actually -- one of the things that slowed me down when testing was block selection -- so I'd love to see a followup for that too. It wouldn't block this initiative, I don't think. @droplet, can you file an issue and link it here?
    
5. On the whole, I'd really appreciate incorporating my suggestion of adding unaddressed points of feedback to the IS (even if they're descoped) in the summary would have been helpful. Until we do that, I feel pressure to read the entire issue and understand whether increasingly outdated feedback will be addressed now, later, or never. E.g. #62 which it looks like Tim considers a Settings Tray issue, but I see no followup for it in the summary. Part of what committers do when reviewing an issue is confirm that others' feedback has been addressed so it would be really helpful if that could happen each time the initiative team processes the feedback.
    

I'm also untagging for Framework Manager review because @larowlan has signed off in #96!

I'm happy with the architecture, but as discussed with Tim via irc/slack - would be keen to see what the data model will look like for #2922033: Use the Layout Builder for entity view_modes

Really looking to reviewing this more when some of the must-have followps around the interactions are in.

Sorry, forgot a few things after I noticed #62 seemed to be missing and got distracted by the unaddressed feedback question.

1. I would suggest that the Layout Builder start with only the main content block in a single-column layout. This really would have made a difference for me in terms of understanding what to do on the page. This also can be a followup; I'd say it's a should-have unless the product and UX teams say it's a must have.
    
2. I think we need an issue to discuss how to make it clear to users how they can get rid of the sidebar and header areas when they're laying out the whole page with Layout Builder. This could be something like a theme with just one single-column region, and a form for Layout Builder that says "Layout Builder can be used to arrange the main content region of your site on the page. For the Layout Builder to control the whole page, you can enable the Layout Builder theme. (This will hide any blocks that were already placed outside the main content area.) This is way out of scope and will need discussion about what exactly we should do, but I think we should put it on the roadmap since it's frustrating to only be able to lay out the white square of the page and to not be able to get rid of the dumb search block on the side or to rearrange things WRT the page top.

Hi Guys
I've played a little bit with Layout Builder today, especially replayed a case with custom blocks
probably you know some plugins like blocks use "third_party_settings", what is pretty useful for implementing additional behavior or data handling for blocks (and for other plugins also).

For example, many site owners use Block Class module to have an opportunity to add extra classes to their blocks according to different front-end rules coming from different frameworks like Bootstrap.

Block Class (probably like other extenders) use "third_party_settings" array as a part of config form of the block
What do you think, would be useful to include "third_party_settings" handling for blocks in Layout Builder?
or should it be an additional/standalone issue about that?

Adding review credits for those who helped shape the final patches

Discussed change record with tim on slack

cross post

Committed 88a22dd and pushed to 8.5.x.

On with the next steps!

Un-postponing all the follow ups and publishing the change record.

EXCITING 🎉

HELL yes. :D

Wow!!! Great job here!!

Do we have specific issues for Messages, User login, Tabs, Primary admin actions blocks failing to be added via the layout builder?

Had to revert this due to #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD which is unfortunately too serious a disruption to leave in HEAD. It's worth considering a patch without the failing part of the test since this is an experimental module, and just adding the test in #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD once we fix it.

I think since this is an experimental module we can commit it with the test without the random failure. There is already a follow up to fix this #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD

This patch removes Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderTest and Drupal\Tests\layout_builder\FunctionalJavascript\ PageReloadHelperTrait

Looks good to me!

Eclipse

Alright, recommitted after reading the diffstat. Thanks @tedbow and @EclipseGc!

Where do we track progress towards marking layout-builder as stable?

@andrewmacpherson, on this issue I believe: #2884601: Add a Layout Builder to core