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
Comment | File | Size | Author |
---|---|---|---|
#4 | Screen Shot 2018-01-16 at 11.24.29 PM.png | 171.82 KB | Kristen Pol |
#2 | 2937159-layout_drag-2.patch | 6.47 KB | tim.plunkett |
Comments
Comment #2
tim.plunkettUnfortunately, 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.
Comment #3
GrandmaGlassesRopeMan👍 for getting rid of this.
JavaScript changes look good to me.
Comment #4
Kristen PolTested the patch and confirmed:
Nitpick: Could change didn't and add comma, e.g.
If the block did not leave the original delta, use the destination.
Nitpick: Could remove empty line.
Comment #5
Kristen PolTo be more clear, item two above is for this part.
(between deltaTo and deltaFrom)
Since the two items above are very small nitpicks, I'm marking RTBC.
Comment #6
GrandmaGlassesRopeMan@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.
Comment #7
tim.plunkettYep. The real code looks like this:
Comment #8
Kristen PolOh... 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. :)
Comment #10
webchickOk, @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!
Comment #12
tim.plunkettComment #13
imclean CreditAttribution: imclean commentedApologies 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.