[PP-1] Reorder Layout Builder sections

I agree 100% this is a need. The current workaround is to 1) create a new section, 2) manually copy any section settings, and 3) move blocks to the new section in the same configuration.

I don't think the backend portion should be overly challenging. The difficulty will be designing an [accessible] UI. IMHO, the drag and drop interface used for blocks won't work well. It would be challenging in the same way that reordering in complex, nested Paragraphs is challenging (i.e. the contents of the page are significantly taller than the browser viewport and you end up with an unwieldy interface).

The block move interface may be a better example to mimic. When section re-ordering is activated, the settings tray opens and and the sections in the body are labeled "Section 1", "Section 2", etc. Instead of re-ordering blocks, you're re-ordering sections:

A patch to get the ball rolling on this issue. No tests written yet.

I created MoveSectionsForm based on the MoveBlockForm as suggested by Chris. Anyone have a better way to sort and re-insert the sections on form submit? What I've got seems to work at least.

I've added a "Move sections" link to the actions for DefaultsEntityForm and OverridesEntityForm. Maybe the link should be somewhere else and/or a trait like PreviewToggleTrait?

Closing #2956204: Allow Layout Builder sections to be reordered as a duplicate, even though it long pre-dates this issue.

@tim.plunkett - Good catch. We somehow missed the original issue.

Review of patch #4:

@cantrellnm - Thanks for creating this patch. It works in my manual testing.

Action label

I would suggest we consider using "Re-order sections" as an action label as it is more descriptive/precise than "Move sections".

I'll defer to someone with a UX background, but I imagine the patch will get some feedback on how the link (or maybe change to button) is presented to editors:

Tab Focus with Keyboard Navigation

When an editor is using keyboard navigation and activates "Add section" or "Add block", the tab focus is moved to the off-canvas element when it is opened. When an editor is using keyboard navigation and and activates "Move sections", the focus is unchanged. They have to tab through the rest of the page before getting to the off-canvas element.

Code standards

Coder returns the following re coding standards:

FILE: ...er-section/web/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 182 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced
     |         |     with use statements
 197 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: 'layout-builder-move-sections'
 198 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: ]
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...rder-section/web/core/modules/layout_builder/src/Form/MoveSectionsForm.php
--------------------------------------------------------------------------------
FOUND 10 ERRORS AND 3 WARNINGS AFFECTING 11 LINES
--------------------------------------------------------------------------------
   1 | ERROR   | [x] The PHP open tag must be followed by exactly one blank
     |         |     line
   2 | ERROR   | [x] Namespaced classes, interfaces and traits should not begin
     |         |     with a file doc comment
  16 | WARNING | [x] Unused use statement
  16 | ERROR   | [x] When importing a class with "use", do not include a
     |         |     leading \
 121 | ERROR   | [x] Data types in @var tags need to be fully namespaced
 124 | ERROR   | [x] Expected 1 space after FOREACH keyword; 0 found
 135 | ERROR   | [x] There should be no white space before a closing "]"
 135 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: ]
 143 | ERROR   | [x] Expected 1 space after "-"; 0 found
 148 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: ]
 180 | ERROR   | [x] Expected 1 space after FUNCTION keyword; 0 found
 227 | ERROR   | [x] Expected 1 blank line after function; 0 found
 228 | ERROR   | [x] The closing brace for the class must have an empty line
     |         |     before it
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 13 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

FILE: ...r-section/web/core/modules/layout_builder/src/Form/OverridesEntityForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 218 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced
     |         |     with use statements
 233 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: 'layout-builder-move-sections'
 234 | WARNING | [x] A comma should follow the last multiline array item.
     |         |     Found: ]
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Separate UI is good for accessibility but is there any reason to not allow dragging of an entire section, anyway we have a flat structure so why to not provide a drag-n-drop for sections?

From #2:

IMHO, the drag and drop interface used for blocks won't work well. It would be challenging in the same way that reordering in complex, nested Paragraphs is challenging (i.e. the contents of the page are significantly taller than the browser viewport and you end up with an unwieldy interface).

A drag-and-drop interface might be workable if we could minimize section heights: 1) hide their blocks (or at least toggle off content preview), 2) hide "Add section" elements, and 3) hide "Add block" elements. This would increase the complexity of the feature request, however.

@Webbeh It looks like the MoveSectionsForm got dropped out of patch #8, and the interdiff is compared with the latest Drupal commit instead. (As a result this interdiff is for patch #4.)

Action label & Code Standards

This patch addresses the code standards issues and changes "Move" to "Reorder" in the form as well. (I have a preference for "Reorder" over "Re-order" but that can be changed of course.)

I'm also unsure of the best place for the "Reorder sections" link and defer to any UX experts, but for now I've changed it to be styled as a button beside the others.

Tab Focus

I'm not sure why focus isn't changing to the dialog after it opens. I found suggestions that using the data-disable-refocus attribute might help, but it didn't work when I tried it. Any ideas for how to fix this?

This patch looks really good. A few items for feedback:

Action label

Re the action label - Visually, I do like it better as a button. And it is an action that will affect the entire display. Also, I agree it should be "reorder" instead of "re-order". (Fixed in the issue title.)

Tab focus

Re tab focus, give this a try in DefaultsEntityForm and OverridesEntityForm. It worked for me:

    $actions['move_sections'] = [
      '#type' => 'link',
      '#title' => $this->t('Reorder sections'),
      '#url' => Url::fromRoute('layout_builder.move_sections_form',
        [
          'section_storage_type' => $this->sectionStorage->getStorageType(),
          'section_storage' => $this->sectionStorage->getStorageId(),
        ],
        [
          'attributes' => [
            'class' => [
              'use-ajax',
              'button',
            ],
            'data-dialog-type' => 'dialog',
            'data-dialog-renderer' => 'off_canvas',
+           'data-disable-refocus' => 'true',
          ],
        ]
      ),
      '#id' => 'layout-builder-move-sections',
    ];

('true' as a string works. true as a boolean doesn't)

Form Title

I missed this on my last review, but I think we should consider dropping the host entity's label from the MoveSectionsForm title. The 'Reorder sections' action is already scoped to this particular display, and we don't need it as context.

With entity title:

Without entity title:

That would involve removing the MoveSectionsForm::title() method and hardcoding the title in layout_builder.routing.yml:

layout_builder.move_sections_form:
  path: '/layout_builder/move/sections/{section_storage_type}/{section_storage}'
  defaults:
+   _title: 'Reorder sections'
-   _title_callback: '\Drupal\layout_builder\Form\MoveSectionsForm::title'
    _form: '\Drupal\layout_builder\Form\MoveSectionsForm'
  requirements:
    _layout_builder_access: 'view'
  options:
    _admin_route: TRUE
    parameters:
      section_storage:
        layout_builder_tempstore: TRUE

Drag-and-drop interface

I revisited a potential drag-and-drop interface with our UX designer, and we couldn't figure out a good way to make it work.

1. A Layout Builder-enabled page populated with content is probably significantly taller than the viewport, which makes drag-and-drop challenging - and we're dragging around entire sections, not just blocks
2. We could add a 'Collapse sections' toggle, similar to 'Show content preview' toggle. This would address the content height vs viewport height issue and make drag-and-drop easier.
3. But, if we collapse the sections, now a user cannot see what is being moving around; the user can only see the title of the section and not any of the content, which is a degraded editing state.

Unless there's a workable solution for drag-and-drop (that doesn't significantly increase the complexity of this patch), my vote is stick with the UI provided in the current patch.

Tab Focus

Thanks, Chris! I had tried setting data-disable-refocus to true but not 'true', so that's fixed now.

Form Title

The form title has been hardcoded to 'Reorder sections' as suggested.

Drag-and-drop interface

I agree we should stick with the current patch's form for now. An additional UI would be complicated enough I think it deserves its own issue and discussion (without delaying this basic implementation of a useful feature).

Tests continue to pass, so we haven't broken anything. At this point, I think we're ready to update unit tests to test the new reorder section functionality. It probably needs it own test class - mabye Drupal\Tests\layout_builder\FunctionalJavascript/MoveSectionFormTest.

I've created Drupal\Tests\layout_builder\FunctionalJavascript\MoveSectionsFormTest as suggested (heavily based on MoveBlockFormTest).

While doing so I thought about the discrepancy between the "Show/Hide row weights" button and using "Delta" instead of "Weight" for that value in the table. I did that because Section doesn't have a weight property like SectionComponent does, but maybe it should.

Any opinions on that? I'm not sure what other repercussions adding weight to Section might have.

Added $defaultTheme to the test to fix the deprecation notice.

Hi,

I have reviewed the patch #20 and it worked as designed.

Below are my updates.

1 it's added the button "Reorder sections" on the layout page.

2 When I clicked on this button pop-up open with section list to change the order of section.

3 After saving the "Reorder sections" form it changed the order of section.

I have also run the included test in the patch.

Below is the output screenshot of the patch.

The patch is working great.

Thanks,
Ren

Thanks for the work on this so far!

This needs a post_update function to clear caches in light of the new routes and UI elements

1. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -171,6 +172,28 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +            'data-disable-refocus' => 'true',
   
   +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -207,6 +208,28 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +            'data-disable-refocus' => 'true',
   

   This is the default value and does not need to be specified here.

2. +++ b/core/modules/layout_builder/src/Form/DefaultsEntityForm.php
   @@ -171,6 +172,28 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      '#id' => 'layout-builder-move-sections',
   
   +++ b/core/modules/layout_builder/src/Form/OverridesEntityForm.php
   @@ -207,6 +208,28 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +      '#id' => 'layout-builder-move-sections',
   

   No need for an #id here

3. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +    $parameters = array_slice(func_get_args(), 2);
   +    foreach ($parameters as $parameter) {
   +      if (is_null($parameter)) {
   +        throw new \InvalidArgumentException('MoveSectionsForm requires all parameters.');
   +      }
   +    }
   

   This is unnecessary. If you must, you can assert($section_storage instanceof SectionStorageInterface);, but even that is overkill IMO

4. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +            'id' => 'layout-builder-sections-table',
   +            'class' => ['layout-builder-sections-table'],
   

   Why duplicate the class and ID?

5. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +    /** @var \Drupal\layout_builder\Section[] $sections */
   +    $sections = $section_storage->getSections();
   

   This @var is unnecessary.

6. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +        '#attributes' => ['class' => $row_classes],
   +        'label' => $label,
   +        'delta' => [
   

   Can you mix and match # and non-# keys like this? I don't recall this working

7. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   +
   +    $new_deltas = $this->getNewDeltas($form_state);
   ...
   +    }
   +
   +  }
   

   Nit: no blank line

8. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +      $deltas = array_combine(array_keys($new_deltas), array_column($new_deltas, 'delta'));
   +      asort($deltas);
   +      $order = array_keys($deltas);
   +      $sections = array_map(function ($delta) use ($sections) {
   +        return $sections[$delta];
   +      }, $order);
   

   That's quite a few array_* functions in a row, perhaps a comment would clear up what is happening here.

9. +++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
   @@ -0,0 +1,210 @@
   +      return (array) $form_state->getValue('sections');
   

   Why the need for the cast?

10. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
    @@ -0,0 +1,238 @@
    +  protected $defaultTheme = 'classy';
    

    Is classy really required here? If not, stark should be used.

11. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
    @@ -0,0 +1,238 @@
    +  protected function setUp() {
    ...
    +    $assert_session = $this->assertSession();
    

    Shouldn't be performing asserts in a setUp method. (I realize this may have been copied from somewhere else, but if so that place itself is wrong)

12. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
    @@ -0,0 +1,238 @@
    +    $keys = [
    +      'up' => 38,
    +      'down' => 40,
    +    ];
    

    This is duplicated now, is there a better way to genericize this?

Re #23.1 - If 'data-disable-refocus' => 'true' is removed, then the tab focus is not moved to the off-canvas element when it is opened. See #7 (Tab Focus with Keyboard Navigation), #11 (Tab Focus), #14 (Tab focus), and #15 (Tab Focus).

Sorry, I misread RenderElement. It has this:

    if (!empty($element['#ajax']['disable-refocus'])) {
      $element['#attributes']['data-disable-refocus'] = "true";
    }

In this case, the LB code should switch to using the #ajax part. Then it can use TRUE instead of the internal implementation detail of string "true".

General comment first: Because MoveSectionsForm was adapted from Drupal\layout_builder\Form\MoveBlockForm, this code review turned into a bit of a code review on MoveBlockForm's existing code, too.

Post update function is added.

1. The proposed change doesn't work. RenderElement::preRenderAjaxForm() does set $element['#attributes']['data-disable-refocus'] = "true"; however, this is ignored when Drupal\Core\Render\Element\Link::preRenderLink() renders $element['#markup']. No changes made in patch.
2. IDs are removed.
3. Designated code is removed. That said, see MoveBlockForm. That's where this code came from. Follow-up issue for MoveBlockForm?
4. The ID is needed for the "Show row weights" functionality in the draggable table. If it's removed, then the JS toggle breaks, and row weights are always shown. The class is used for CSS. No changes made in patch.
5. @var comment is removed. There's a comment in MoveBlockForm that this one was adapted from. Follow-up issue for MoveBlockForm?
6. This code came from MoveBlockForm, and so far as I can tell, it functions correctly. No changes made in patch.
7. Blank line is removed.
8. Comments added.
9. I don't think the cast is necessary either. Cast is removed.
10. Theme is changed to 'stark' and tests still pass locally. Code is from MoveBlockForm. Follow-up issue for MoveBlockForm?
11. I moved most of the setup code to ::testMoveSections(). There's only one test. Code is from MoveBlockForm. Follow-up issue for MoveBlockForm?
12. I'm only seeing one instance in MoveSectionsFormTest. Do you mean re-use across tests? I'm seeing duplicate code in Drupal\Tests\layout_builder\FunctionalJavascript\MoveBlockFormTest::moveBlockWithKeyboard() and near-duplicate code in Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest::moveRowWithKeyboard(). We could add a Trait to core's TableDrag test. Please advise. (No changes made in patch yet)

Updated patch with path fixed.

This continues coming up, bumping to major.

Updating the patch file for the latest version of core

Correcting that patch file

Third time lucky

@CarlHinton - could you post an interdiff?

Adding patch 34 - which fixes some addional coding standard errors within Layout Builder - and also got this to work for me using Core 8.8.5
Also adding interdiff file

Looks like I did it again - I left in my www/ path

Oh dear - this is embarising

@CarlHinton - Can you provide some context for the changes you're making? For example, \Drupal\layout_builder\Form\MoveSectionsForm has been removed; however, the 'layout_builder.move_sections_form' route still defines it as the Form class to return. Also - why was test coverage removed?

@Chris Burge - thank you for pointing this out to me, the Form should still be added - but for some reason the git diff has completely removed it - very annoying - I will see if I can get that back in

Attached is a re-roll of #28. (#28 still applies to 9.1.x - just with fuzz on two files).

It looks like we're getting a deprecation notice. I'll post an updated patch tomorrow.

Updated patch attached.

Patch applies without fuzz and tests pass.

Here's a summary of where we are with the review process (review in #23 and response in #27).

With the exception of the final item, I just need to know if we need to file a follow-up issue for MoveBlockForm. For the final issue, I need clarification.

3: Follow-up issue for MoveBlockForm?

Review comment from #23:

+++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
@@ -0,0 +1,210 @@
+    $parameters = array_slice(func_get_args(), 2);
+    foreach ($parameters as $parameter) {
+      if (is_null($parameter)) {
+        throw new \InvalidArgumentException('MoveSectionsForm requires all parameters.');
+      }
+    }

This is unnecessary. If you must, you can assert($section_storage instanceof SectionStorageInterface);, but even that is overkill IMO

5: Follow-up issue for MoveBlockForm?

Review comment from #23:

+++ b/core/modules/layout_builder/src/Form/MoveSectionsForm.php
@@ -0,0 +1,210 @@
+    /** @var \Drupal\layout_builder\Section[] $sections */
+    $sections = $section_storage->getSections();

This @var is unnecessary.

10: Follow-up issue for MoveBlockForm?

Review comment from #23:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
@@ -0,0 +1,238 @@
+  protected $defaultTheme = 'classy';

Is classy really required here? If not, stark should be used.

11: Follow-up issue for MoveBlockForm?

Review comment from #23:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
@@ -0,0 +1,238 @@
+  protected function setUp() {
...
+    $assert_session = $this->assertSession();

Shouldn't be performing asserts in a setUp method. (I realize this may have been copied from somewhere else, but if so that place itself is wrong)

12 I'm only seeing one instance in MoveSectionsFormTest. Do you mean re-use across tests? I'm seeing duplicate code in Drupal\Tests\layout_builder\FunctionalJavascript\MoveBlockFormTest::moveBlockWithKeyboard() and near-duplicate code in Drupal\FunctionalJavascriptTests\TableDrag\TableDragTest::moveRowWithKeyboard(). We could add a Trait to core's TableDrag test. Please advise. (No changes made in patch yet)

Review comment from #23:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/MoveSectionsFormTest.php
@@ -0,0 +1,238 @@
+    $keys = [
+      'up' => 38,
+      'down' => 40,
+    ];

This is duplicated now, is there a better way to genericize this?

Just reviewed this as an editor. It works very well.
But I got lost a bit when I had applied a new ordering, did not save yet, and then opened the reorder-dialog a second time.

To improve usability I would like to suggest:

• a. Either show both the 'before save' and the 'new' position in the dialog. This should happen when you open the dialog a second time with unsaved changes (changed ordering).
• b. Or consistently use the 'before save' naming. So when you open the dialog a second time with unsaved changes, the initial order might be 'section 2, section 1, ...'. IMO this would work well because the user did not save yet and thus expects references to the old state
• When the order has changed, show the ordering-position as a label (section 1, section 2) on the sections overview
• When you open the reordering-dialog a second time (with unsaved changes) show a 'reset' button that reverts to the 'before save' positions
• When you open the reordering-dialog a second time (with unsaved changes) show a 'cancel' button that keeps the current changes but doesn't apply new ordering

After saving a new revision will be created. Thus the 'before save' and 'new' should only be there when "You have unsaved changes" is shown to the user.

A different (or additional) UX pattern that would be helpful is with up- and down-icons next to the gear and cross icon on each section.

See this attached image with a mockup:

With this pattern a cancel- or revert- buttons specific for reordering are not needed, because the editor can use the standard 'Reject changes' button.

@ndf - Thanks for testing out this patch and for providing feedback. I agree the UX could be improved; however, I'm not sure if that's in scope for the current issue. The UX recommendations also apply to moving blocks around (not just sections). I'll defer to a subsystem maintainer.

Hi @Chris . Most UX remarks are indeed nice to have. But I would like to advocate for

b. Or consistently use the 'before save' naming. So when you open the dialog a second time with unsaved changes, the initial order might be 'section 2, section 1, ...'. IMO this would work well because the user did not save yet and thus expects references to the old state

Because that remark seems most simple to implement and will make the current patch easier to use.

#43 works perfectly when applied to D9.07. Thanks!

#43 is working very fine even with Drupal v8.9.x. Thanks!

Base on #43, changing to Needs work.

Based on what in #43 @xmacinfo? Can you be more specific? Maybe that is the wrong comment number...? The user who set this to "Needs review" did so in comment 43.

Sorry, I meant #44

Setting to 'Needs Review'. There's no work to be done without a subsystem maintainer review. Everything in #44, with one exception, is asking if we should create a follow-up issue for MoveBlockForm. The remaining item is minor and deals with duplicate code.

#43 works perfectly. Thanks!

Installation details:

Drupal 9.1.4 Installation with default language DE and those modules installed:

1. drupal/bootstrap_layout_builder
2. drupal/layout_builder_at
3. drupal/layout_builder_modal

Also tested with EN translation.

The patch in #43 applied cleanly to Drupal 9.2.0 and looks to work well.

Install includes modules

bootstrap_layout_builder 2.0.1
bootstrap_styles dev-1.0.x 6ed42a2
layout_builder_modal 1.1.0
section_library 1.0.0-beta2

I am using this patch successfully for a few months now. Changing to RTBC to get more eyes on it after the few comments above that the points in #44 were about potentially future work...?

We still need a subsystem maintainer review as per #55 and the patch does not apply to 9.3.x.

Working on re-roll.

drupalPostForm() is deprecated. Instead, use drupalGet() on the path, and then $this->submitForm(['layout[enabled]' => TRUE], 'Save');

Also tagging for usability review.

How does this issue get a subsystem maintainer to review it? I'm just coming back to this one... will give #66 a try and report back if there are any issues, but as I mentioned previously we have been using this patch for almost a year now with great results.

Just to add "pressure": tested and works like a charm on a 9.3.9 install, this should be commited to core as soon as possible, is a very much missing feature of Layout Builder.

Based on previous 2 comments, marking as RTBC.

Re-rolled for 9.4.x

I have applied patch #71 for drupal 9.4.x-dev.The patch looks good to me . Adding screenshots for the reference.

We might review this issue at #3291116: Drupal Usability Meeting 2022-06-24, in about 30 minutes.

Is anyone available to walk us through the changes on Zoom?

We discussed this issue at #3291116: Drupal Usability Meeting 2022-06-24. That issue will have a link to a recording of the meeting.

For the record, the participants in the usability meeting were AaronMcHale, rkoller, tim.plunkett, worldlinemine, and me.

@tim.plunkett will post a comment with a summary of the discussion.

I joined the UX team to discuss this approach.

The off-canvas tabledrag approach is great, there are some tweaks and improvements to be made.

1. Move the "Reorder sections" trigger to be in-line with each section, alongside the remove X and the "Configure section" link. This is inspired by http://www.drupal.org/project/lb_ux.
   Reasons for this include: the "reorder sections" is something likely done while looking at a given section, and scrolling back to the top takes you away from that focus. Also, the "Save layout" and "Discard buttons" are actual buttons that perform a standard operation, and it's easy to scroll past that and miss the "Reorder sections" link
2. By having the reorder section be triggered from a specific section, we can highlight that section in the tabledrag. Compare to the highlight effect when you use the "Move" contextual link for any block in Layout Builder.
   See also the Block UI, after placing a new block, it scrolls to and highlights the newly placed box.
3. Add a second column to the tabledrag with the human-readable label of the layout plugin used for each section.
   This additional metadata will be very helpful when trying to keep track of which section is which, especially if you haven't added custom labels to your sections (by default they are only numbered)

   If needed, it should be possible to specify a greater width for the off-canvas sidebar.

I'll let @benjifisher remove the tag, but that is my understanding of our consensus. Thanks!

Someone please assign the issue to me once this is implemented so I can provide the subsystem maintainer signoff.

I notice that there is a contrib project Layout Builder Reorder. It does not have any releases, but it does have some code. I guess it will be obsolete when this issue is committed.

I think that #77 is a good summary of the usability meeting last week. Let me add a couple of points.

Try to make the UI for reordering sections as much like the UI for reordering components as you can. That applies to the interaction in the off-canvas sidebar. It also applies to the link that opens the sidebar. For reordering components, I think that is a contextual link, along with Configure and Delete. Would it make sense to have similar contextual links, or a drop button, for sections?

I am removing the tag for a usability review and adding one for an issue summary update. I see links to some screenshots in #74, but nothing under "User interface changes" in the issue summary. Once the suggestions in #77 are implemented, it will help to get updated screenshots and add them to the issue summary.

I am also adding the tag for a follow-up issue. In addition to the mechanism of this issue, using the off-canvas sidebar, I think there should be a click-and-drag option, just as there is for components. As part of the follow-up issue, we should provide a way to collapse sections: perhaps replace the "Show content preview" checkbox with a set of radio buttons with three options (section outline, component outline, content preview).

I am adding #2995689: Allow reordering blocks without a pointer device as a related issue. It was helpful to review that during the usability meeting last week.

I think this issue needs a change record, so I am adding the tag for that. It might also need a release-note snippet at the end of the issue summary.

I'm glad to see this issue moving forward. If we're going to put a 'reorder sections' link on each section, it would make sense to use a contextual link for that.

+1 for spinning off a couple of follow-up tasks.

Hey guys newbie to Drupal here, can anybody help me installing the patch?
I researched and fond that to install patches they go here:

 "patches": {
            "": {
              
            },
            "dmore/chrome-mink-driver": {
                "https://github.com/acquia/blt/issues/3224": "patches/drupal-extension/chrome-mink-driver.patch"
            },
            "drupal/core": { }
}

Do I have to add the patch reference to drupal/core or what module/name should I put there?

#71 is working well for us in Open Y distro

Would it be possible to somehow re-order the sections without using the action button at the top? This way compatibility with modules like Layout Builder Widget (moves layout builder form to entity edit page) would be possible.

Thanks, For having this issue and the patch for ~9.0 .
Noted, that the patch is not applying to the 10.0.x branch

Updated patch for 10.0.x/10.1.x.

@alexahumada can you please provide an interdiff and how the previous patch no longer applies per the new policy. But definitely an interdiff as the file size goes up from the previous patch of #71

Also per #90 there are still some remaining tasks.

Hi @smustgrave,

sorry i forgot about the new interdiff policy.

The old patch can't be applied to Drupal 10 because some changes in code in 10.0.x and 10.1.x branches, notably some css classes present in #71 patch have been removed. Also, difference in size is caused by Drupal 10 using PostCSS to transpile pcss files to css using yarn, which require changes to new .pcss.css files.

I'll try to work on the remaining task we have in #77 and #85, but sadly this new patch only addresses Drupal 10 compatibility.

The patch from #72 still applies to D11, yay!

Updated the patch to fix the CCF and updated the tests since they were failing.

Setting to needs review, if tests come back green I'll jump onto IS and the CR.

I've put #100 in https://git.drupalcode.org/project/drupal/-/merge_requests/4075

Let's make our lives better with MR

Could issue summary and follow up tags be removed based on #86.

Also the change record?

Think the MR failures are random so running the tests again.

Updated the IS to add some screenshots of the functionality.

Added a draft CR. Happy for review and more information added as needed.

Thanks for the quick updates.

Tested out a little by adding 3 sections.
The reorder work but noticed something weird
If I change the sections to say 3,1,2 it renames the sections to 1,2,3.
It moves the content in the blocks correctly but was a little confusing.

Is that desired behavior?

Personally I would find it more confusing if sections were arbitrarily numbered based on their starting position rather than sequentially down the page.

So is more about bulk moving blocks vs sections right? Since the sections rename

Not necessarily, sections can have different layouts/number of columns. But if you think of a section as a 'group of blocks' then sure, it's about moving the blocks (but also the layout containing them).

I think it makes sense to keep them numbered based on how they are stacked on the page at the time you are re-ordering sections, otherwise it becomes a meaningless label.

I agree with this

CR looks good
MR tests are passing

Someone please assign the issue to me once this is implemented so I can provide the subsystem maintainer signoff.

from #77

Will take a look either tomorrow or at DrupalCon

Setting back to NR for @tim.plunkett to give his thoughts as subsystem maintainer as per #114

I've updated the MR to address the first of the remaining tasks: Moving the Reorder trigger so it's inline along with the Remove and Configure triggers for each section.

• Removed action from Default and Overrides entity forms
• Added action to each section via the Layout Builder element
• Added minimal CSS to separate links

I started to take a look at the 2nd task, highlighting the section in the off-canvas tabledrag now that we know which section the request was called from. In the Move Blocks UI, the block is identified by its UUID. Layout sections do not have UUIDs (see #3208766: Add UUID to sections). If the section does not have a label, the only way to distinguish it is by its delta. Is a highlight going to be doable with just the delta?

Maybe this is blocked by #3208766: Add UUID to sections

looking at this again I feel like it should be postponed on #3208766: Add UUID to sections but if anyone disagrees please put back to NR.

Sounds right to me, let's make it official