Problem/Motivation

Layout builder works great for layouts with multiple regions, but when dragging components from one delta in a given region to another delta in the same region, layout builder's js doesn't even send an ajax request to inform Drupal of the event.

If you drag to the beginning of a region, it places it at the end of the region.

If you click the "Add Block" link at the top of a region, it adds it to the end.

Proposed resolution

Send AJAX for components being moved within a single region, regardless of where they are dragged to within that region.
Move the "Add Block" link to the bottom, as per the comps.

Remaining tasks

DO IT!

User interface changes

The UI will work as expected rather than just not working.

API changes

none

Data model changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc created an issue. See original summary.

tim.plunkett’s picture

Title: Layout Builder doesn't properly drag/drop for components in a single region. » Manipulating placement of components in Layout Builder UI is inconsistent
Issue summary: View changes
Status: Active » Needs review
Issue tags: +JavaScript
FileSize
6.47 KB

Unfortunately, we cannot write tests for this until #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD is fixed.

Committers, please add credit to @drpal for helping with the JS.

GrandmaGlassesRopeMan’s picture

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -14,15 +14,19 @@
-                $(this).data('region'),

👍 for getting rid of this.

JavaScript changes look good to me.

Kristen Pol’s picture

Tested the patch and confirmed:

  1. Add block has been moved to the end
  2. Moving around components works as expected
  3. Moving between sections still works
  1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
    @@ -14,15 +14,19 @@
    +            // If the block didn't leave the original delta use the destination.
    

    Nitpick: Could change didn't and add comma, e.g.

    If the block did not leave the original delta, use the destination.

  2. +++ b/core/modules/layout_builder/js/layout-builder.js
    @@ -16,9 +16,13 @@
    +
    

    Nitpick: Could remove empty line.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

To be more clear, item two above is for this part.

+++ b/core/modules/layout_builder/js/layout-builder.js
@@ -16,9 +16,13 @@
+            var deltaTo = ui.item.closest('[data-layout-delta]').data('layout-delta');
+
+            var deltaFrom = ui.sender ? ui.sender.closest('[data-layout-delta]').data('layout-delta') : deltaTo;

(between deltaTo and deltaFrom)

Since the two items above are very small nitpicks, I'm marking RTBC.

GrandmaGlassesRopeMan’s picture

@Kristen Pol

Looks like you're looking at the transpiled version of the JavaScript. The reason the space appears is because we remove comments in the transpile process. However, it appears that depending on how the comment is structured it removes the empty line.

~~~

I'll probably take a look at this behavior at some point.

tim.plunkett’s picture

Yep. The real code looks like this:

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -14,15 +14,19 @@
+            // Find the destination delta.
+            const deltaTo = ui.item.closest('[data-layout-delta]').data('layout-delta');
+            // If the block didn't leave the original delta use the destination.
+            const deltaFrom = ui.sender ? ui.sender.closest('[data-layout-delta]').data('layout-delta') : deltaTo;
Kristen Pol’s picture

Oh... gotcha. I see what you mean... then the only super nitpick is the one comment but I wouldn't hold this up for a comma. :)

  • webchick committed 06343ef on 8.6.x
    Issue #2937159 by tim.plunkett, Kristen Pol, EclipseGc, drpal:...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, @tim.plunkett walked me through the before/after on this patch + the code.

Looks good here. Nice to relocate the "Add" button closer to where the action is, also nice that you can now re-order things and they stay where you put them. ;) One thing I noted was lack of test coverage, but Tim pointed out that #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD is still an open issue so not an option right now.

Committed and pushed to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Component: layout.module » layout_builder.module
imclean’s picture

Apologies for commenting in an old issue, but this seems like quite an important bug fix. I understand in 8.5 layout_builder is experimental, but is there a reason this couldn't be backported to 8.5? Being able to specify the order in which content is displayed in a region is fundamental to how the feature should work.