Normalize block place and outside-in experiences

This looks like part of (or a duplicate of?) #2739079: Reduce visual clutter of Place Block module.

Ok here is patch that changes block_place to use offcanvas if outside_in is enabled. This should probably be changed the "block_place" component when available.

It currently works(I like this much better!) but probably should add some js/css to highlight which region the user just clicked. In the modal it doesn't really matter as much but with offcanvas it would be a nice change. Should that be a follow-up?

Ok here is a patch that includes the work in #2786459: "Offcanvas" tray should be using the existing dialog system. It is RTBC.
Also a patch that shows just changes to block_place

@tkoleary(and everyone) #2786459: "Offcanvas" tray should be using the existing dialog system just landed so here is re-rerolled patch

Postponing this because of #2804639: Offcanvas dialog width not respected

This patch needs to be able to set the width.

#2804639: Offcanvas dialog width not respected was fixed!

Current patch still applies

Talked with UX team:
We should also make the sort of blocks in the region it was just added to as the last step in the adding process. Open up the tray with the blocks in the region to sort.

Also in follow add a new contextual link for each block that would open tray with blocks in region to sort.

Per #17.

This patch does

Talked with UX team:
We should also make the sort of blocks in the region it was just added to as the last step in the adding process. Open up the tray with the blocks in the region to sort.

Currently you can sort the blocks if you don't use the table drag but instead show block weights.

This is because of #tabledrag settings in \Drupal\block\BlockListBuilder::buildBlocksForm.
Not sure if we should do a form alter and try to setup the table drag to not expect region groups.

Test failed because we now have new route for block library. Fixed

simplytest.me under 8.2
seems to work well.

it does require the outside in to be active as well. i was expecting block place to just be in the tray (but i'm personally unsure of how much the tray codebase is in core) and if it could be called without outside in being active.

Also, has no styling. (again, i'm unsure how much of styling and and any of #2826722: Add a 'fence' around settings tray with aggressive CSS reset. related work should be expected at this point.

it does require the outside in to be active as well. i was expecting block place to just be in the tray

@SKAUGHT yes until #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it you will need Settings Tray module on.

And yes the styling won't apply to all uses of the tray until #2826722: Add a 'fence' around settings tray with aggressive CSS reset.

+++ b/core/modules/block_place/src/Controller/BlockPlaceListController.php
@@ -0,0 +1,55 @@
+/**
+ * Defines a controller to list blocks in sort blocks dialog.
+ */
+class BlockPlaceListController extends BlockListController {

I now think makes the situation overly complex. It is basically fighting it's parent class to make a simple listing of block entities in a region to produce a sortable table.

It would probably be much easier to just make a new class BlockListRegionSorter that loads the blocks and create a simple sortable form.

I am marking this as postponed until https://www.drupal.org/node/2784443 seeks resolution.

This has a high likelihood of simplifying the approach used in the most recent patch.

Issue summary updated to reflect this status.

The dependency on Outside In would be a temporary dependency with a "todo".

Scenario 1: #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it lands, and the dependency to Outside In gets removed. At that point, the tray will be in the regular dialog system and you wont need the module enabled to use it.

Scenario 2: In the event that Outside In doesn't stabilize, we remove and perform some cleanup to work with a modal window. Changing data dialog type to "modal" instead of "off_canvas" (this is a small tweak).

This patch is a significant step forward to finish the user experience proposed in #2739075: [plan] Make Place Blocks module functionality part of the Block module (etc.).

I tested two cases:

Case 1. Outside In enabled, video https://youtu.be/9W7I1jNvJhA

I clicked "Place Block", clicked "+" in a region, selected the block, filled out the settings and clicked save.

The page reloaded and a JS error occurred when I expected the block sorting for the selected region.

Case 2. Outside In not enabled, video https://youtu.be/zrnDk_kMgiU

I clicked "Place Block", clicked "+" in a region, selected the block, filled out the settings and clicked save.

The page reloaded and the block sorting popped up for the selected region. If I clicked "save", it reloaded the page and the block was placed where desired. If I attempted to re-order the blocks before saving, a JS error occurs and I can no longer save.

@nerdstein thanks for testing and posting the videos

re: case 1. because of #2862625: Rename offcanvas to two words in code and comments. we have to rename data-dialog-type from "offcanvas" to "off_canvas"

case2:
Yes the saving was not working. I was trying to extend \Drupal\block\Controller\BlockListController for the block sort form. This was not a good idea 😜

I created a new BlockRegionSorterForm and use it for sorting.

The sorting should work now.

note: the tray will still look plain until #2826722: Add a 'fence' around settings tray with aggressive CSS reset.

Here is short video with Settings Tray enabled: https://youtu.be/6TWemaymcDM

This is looking great, @tedbow.

Without settings tray: https://youtu.be/xpyTNJtnQ-w

With settings tray: https://youtu.be/TWqSlNNbel8

Both cases work great and support the desired user interface outcomes discussed #2735277-16: Let users set the weight of blocks after placing them from place-block and #2739075-27: [plan] Make Place Blocks module functionality part of the Block module (etc.).

The implementation is spot on. The sorting form, implemented as a separate form, would allow for easily "adding a second icon next to or below the plus in regions with > 1 block to allow the blocks to be sorted" as @pwolanin notes.

I am marking this as "Reviewed & tested by the community". I did see, what seems to be, a minor indentation code standard violation in the `block_place_page_top` part of the patch. And, @tedbow, I'd be curious to get your thoughts if this needs any additional automated tests.

@nerdstein thanks for the review again.

I left this comment about sorting on #2739075: [plan] Make Place Blocks module functionality part of the Block module (etc.) but will repeat here:

In the last meeting I had with the UX we had decided that there should be a new contextual link on each block that says "reorder blocks"(or similar). This would open the tray with the blocks in that region to be sorted. This way you won't have to click "Place Block" when you actually want to sort. It would also give you the ability to sort blocks outside of placing. I was going to add this functionality to #2784495: Normalize block place and outside-in experiences

I think this issue needs work/review for

1. Javascript tests for sorting functionality
2. Code standards mentioned in block_place_page_top
3. The sorting via contextual link
4. Is there better solution for than block_place_page_top use?

This adds the "Sort Blocks" contextual links to each block

IMPORTANT: You have to uninstall and reinstall this module for the contextual links to show up. This is because of problem with context links caching #2773591: New contextual links are not available after a module is installed
Since this is an experimental module and turning it on and off does not cause any data lose I think this is ok.

It also has to have JS to make sure the new contextual link will work with ajax. because #2764931: Contextual links don't work with 'use-ajax' links

This patch also adds the new es6.js file needed now for all Javascript

Fixes Code standards mentioned in block_place_page_top

UPDATE:
I had thought about not putting the contextual link in regions that only have 1 block because it doesn't make sense to sort. But I thought that in some themes it would not be obvious 1 one region started and another ended so the new site builder or someone new a particular theme it would not be clear why some blocks had the "sort" link and other didn't.
For instance I think actually in Bartik Footer 1, Footer 2 would be confusing.

I did just realize we could not open the tray for sorting at the end if we are placing a block in a region with no other blocks. It doesn't make sense because they can't do anything.

Added remaining tasks to summary

This patch adds not open the sort tray when placing the only block in a region because there would be no other block to sort by.

i see what you're working on here.. i'ld opened #2887032: Let user set block weight in while configuring block instance steaming from 2735277

Fixes to JS standards

Add Drupal.blockPlace.isBlockPlaceMode() in JS.

This called from outside_in.es6.js to ensure that when "Place Block" mode is entered Settings Tray "Edit Mode" is exited

this is very nice. i was just playing with #38.

one UX note: context of blocks

• i set an extra 'page title' block to Header region
• i set 'site branding' only for
• went to /contact
• clicked 'sort blocks' (while on /contact)
• result: both blocks were in listed in sort table, even though one was off page context

also:
if multiple block instances (of the same block) are present, no way to know "which block instance is which" in the sort table.

It might be worthwhile showing the machine name in a new column (or on hover) to highlight the block instance differences. That, to me, is a block instance differentiation. There may be other less technical attributes we can show.

I checked the patch locally and it doesn't look like it needs a reroll. Moving back to "needs review" and triggering test.

Ah, I see what's going on. I'm testing against 8.4.x locally (which applies cleanly). It looks like 8.3.x does not.

Should this issue be updated to 8.4.x? It's my understanding that is where it would be committed.

I would assume our expected release is 8.4.x. I'm going to update the issue accordingly and confirm the patch applies cleanly. Apologies in advance if this is indeed 8.3.x, for which I'm happy to reroll #40 if needed.

I expected ES6 ready in all new JS files :)

@droplet can you explain what needs to be done?

Sorry I am new to es6. I have updated the block_place.es6.js with a couple es6 features that I know about.

Also I am doing the build step correctly.

Thanks for your help.

@tedbow,

the little changes would help :P

+++ b/core/modules/block_place/js/block_place.es6.js
@@ -0,0 +1,37 @@
+      // If drupalSettings.block_place is set open open dialog.

Do not understand what it is.

+++ b/core/modules/block_place/js/block_place.es6.js
@@ -0,0 +1,40 @@
+            selector: '.ckeditor-dialog-loading-link',

Why selector is '.ckeditor' prefix? Can it work without CKEDITOR module?

OutsideIn doesn't work:

1. node?block-place=1&destination=/drupal8x/node
2. click `+` to open Place block (and then do nothing)
3. click top left Edit to open Setting Tray
4. click any `Setting Tray` area to edit. (setting tray dialog does not shown)

@droplet - the patch in #50 seems to be working well for me.

As of the patch found in #50, hook_page_top is being used to load the JS library needed to sort blocks.

This may be appropriate for the timing of when the JS library should be loaded on the page. However, in my opinion, the context seems off to me. The sort form loads in either a modal or in the settings tray, depending on if the settings tray module is enabled. Neither of these relate to my understanding of hook_page_top.

One alternative would be to consider something like hook_page_attachments, which would more generically load the library and can perform the contextual check for "block-place-region-sort".

Adding a patch that switches this logic to hook_page_attachments for review and consideration.

Like @tedbow, this is my first time using ES6.

I have the following observations based on @droplet's comments on #2784495-50: Normalize block place and outside-in experiences.

1. I was able to remove the ckeditor selector line, clear the cache, and it seems to be working fine. This may not be necessary.

2. block_place.libraries.yml is still referencing `js/block_place.js` and not the block_place.es6.js file. Is this intentional? Would we want to remove block_place.js?

#51 needed to be rerolled b/c of #2880007: Auto-fix ESLint errors and warnings

The only other change I made was #52.1
I think this was a copy/paste error of my from earlier in the patch

Re #52.2
Yes libraries files will never reference the *.es6.js files. The new es6 development process allows us to write in es6 Javascript but then use BabelJS to compile it to regular Javascript that the browsers can understand.

I think the change #51 to use hook_page_attachments is more correct because that is exactly what we are doing, altering attachments.

1. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,39 @@
   +  Drupal.blockPlace = {
   +    isBlockPlaceMode: () => {
   +      const queryString = decodeURI(window.location.search);
   +      return (/block-place=1/i.test(queryString) || /block-place-region-sort=/i.test(queryString));
   +    },
   +  };
   

   I don't think depending on the query string for state is great idea. Could we possibly add something at load `Drupal.blockPlace.isBlockPlaceMode = true`. With that we could get rid of this entire function.

2. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,39 @@
   +          const blockSort = Drupal.ajax({
   

   Don't need to assign this to a variable.

Did changes according to #54. Please check once.
And is there same issue in 'core/modules/block_place/js/block_place.js' ?

@pk188 thanks for the patch.

I am attaching an interdiff file between #53 and your patch. This helps reviewers figure out what is the exact difference between the last patch.
Here are instructions https://www.drupal.org/documentation/git/interdiff

1. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,33 @@
   +  Drupal.blockPlace.isBlockPlaceMode = true;
   

   This should actually be set in drupalSettings and it should be set on the php side.

   Also outside_in.es6.js would need to be changed to use this.

2. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,33 @@
   +        $(window).once('block_sort').each(() => {
   +          Drupal.ajax({
   +            dialog: {},
   +            dialogType: drupalSettings.block_place.dialog_type,
   +            url: drupalSettings.block_place.dialog_url,
   +            progress: { type: 'throbber' },
   +          }).execute();
   

   This changes looks good!

+++ b/core/modules/block_place/js/block_place.js
@@ -0,0 +1,39 @@
+  Drupal.blockPlace = {
+    isBlockPlaceMode: function isBlockPlaceMode() {
+      var queryString = decodeURI(window.location.search);
+      return (/block-place=1/i.test(queryString) || /block-place-region-sort=/i.test(queryString)
+      );
+    }
+  };

The old code is still in the block_place.js file

When editing Javascript now in Drupal core you need to follow this process: https://www.drupal.org/node/2815083

uploading a patch with these fixes.

There need to be test for the new workflow.

+++ b/core/modules/block_place/js/block_place.js
@@ -6,28 +6,16 @@
-  Drupal.blockPlace = {
-    isBlockPlaceMode: function isBlockPlaceMode() {
-      var queryString = decodeURI(window.location.search);
-      return (/block-place=1/i.test(queryString) || /block-place-region-sort=/i.test(queryString)
-      );
-    }
-  };

+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -3,7 +3,7 @@
-(function ($, Drupal) {
+(function ($, Drupal, drupalSettings) {

@@ -194,7 +194,7 @@
-        if (!Drupal.hasOwnProperty('blockPlace') || !Drupal.blockPlace.isBlockPlaceMode()) {
+        if (!drupalSettings.hasOwnProperty('block_place') || !drupalSettings.block_place.isInBlockPlaceMode) {

Could keep this function, and have it do the checking of drupalSettings. Then outside_in.js doesn't have a new dependency on drupalSettings (which isn't in the libraries.yml right now, afaik)

@tim.plunkett that seems like good idea.

I like having other modules call Drupal.blockPlace.isBlockPlaceMode() and not to have to worry how that is determined, whether it is drupalSetting, the query string or some other method.

You're depending on `core/drupal` which has a dependency on `core/drupalSettings`.

+++ b/core/modules/block_place/js/block_place.es6.js
@@ -0,0 +1,37 @@
+  Drupal.blockPlace = {
+    isBlockPlaceMode: () => {
+      if (drupalSettings.hasOwnProperty('block_place') && drupalSettings.block_place.hasOwnProperty('isInBlockPlaceMode')) {
+        return drupalSettings.block_place.isInBlockPlaceMode;
+      }
+      return false;
+    },
+  };

If you are going to use this function, which I don't think you need to, use the object property shorthand.

1. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +  if ($region_sort = $query->get('block-place-region-sort')) {
   ...
   +    $block = Block::load($items['block_configure']['route_parameters']['block']);
   

   Are there cacheability concerns here?

2. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +    $destination = \Drupal::destination()->get();
   +    $destination = explode('?', $destination)[0];
   

   This confuses me, does the destination really have a ? in it?

3. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +  if (isset($element['#links']['block-placesort']) && isset($items['block_configure']['route_parameters']['block'])) {
   +
   

   Extra blank line

4. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +    // Add arguments to sort blocks url.
   

   URL

5. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +    /** @var \Drupal\block\Entity\Block $block */
   

   \Drupal\block\BlockInterface

6. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +    $url = &$element['#links']['block-placesort']['url'];
   

   The & isn't needed, nor is the variable really. Can call ->setRouteParameters directly on the thing

7. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +78,81 @@ function block_place_toolbar() {
   +    $url->setRouteParameters(
   +      [
   +        'theme' => $block->getTheme(),
   +        'region' => $block->getRegion(),
   +      ]
   +    );
   

   can have ([ and ]); together, indenting the middle lines a bit

8. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,37 @@
   +  // Remove in https://www.drupal.org/node/2764931.
   

   Needs an @todo

9. +++ b/core/modules/block_place/src/Controller/PlaceBlockLibraryController.php
   @@ -0,0 +1,46 @@
   + * @package Drupal\block_place\Controller
   

   This isn't a thing

10. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,114 @@
    +    // Make sure the blocks are in the correct order.
    +    usort($blocks, function (Block $blocka, Block $blockb) {
    +      return $blocka->getWeight() - $blockb->getWeight();
    +    });
    

    Please use uasort($assignment, 'Drupal\block\Entity\Block::sort')

11. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,114 @@
    +    /** @var \Drupal\block\Entity\Block $block */
    

    \Drupal\block\BlockInterface[] $blocks

12. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,114 @@
    +    foreach ($form_state->getValue('blocks') as $block_id => $block_weight) {
    ...
    +      $block = $this->entityTypeManager->getStorage('block')->load($block_id);
    

    This should borrow from \Drupal\block\BlockListBuilder::submitForm, by doing one load

13. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,114 @@
    +    drupal_set_message('The block order has been saved');
    

    Missing a full stop

14. +++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
    @@ -96,24 +120,50 @@ public function build() {
    +          $query['destination'] .= "?block-place-region-sort=$region";
    

    This looks very unsafe/incorrect. Destination strings should not have ? in them. This could be tricky to fix...

15. +++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
    @@ -96,24 +120,50 @@ public function build() {
    +        $place_block_route = 'block_place.admin_library';
    ...
    +        $place_block_route = 'block.admin_library';
    ...
    +      $place_block_route = 'block_place.admin_library';
    ...
    +          'link' => Link::createFromRoute($title, $place_block_route, ['theme' => $theme_name], [
    

    We overwrite this variable now?

16. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -193,7 +193,10 @@
    +        // Do turn on edit mode when on block
    

    This comment doesn't parse for me. Also missing a trailing space. Additionally, this adds a soft-dependency in the wrong direction, creating a bit of a circular dependency

I can probably figure the rest out, but some assistance on points 2, 14, 15 from above would be great.

@tim.plunkett I addressed the ones you mentioned in #62 as well as the ones I could do quickly while making the patch.
1. NOT Addressed
2. Yes at this point $destination will equal something like "/d8_2_ux/node?block-place-region-sort=content"
block-place-region-sort is passed in the query string to know which region the block was placed into so we can sort it.
The tray will open to sort this region on this page load. But we want the new destination for submitting the region sort for we don't want to pass block-place-region-sort again or the tray will open again after the user has already sorted.

5. fixed
6. fixed
7, fixed
8. fixed
9. "Thing" is very broad term to declare that some"thing" isn't. But fixed.
10. I didn't that was a "thing". fixed.
11. fixed
12. NOT addressed

14. See 2

15. Yes I guess this left over from when we only needed to use block_place.admin_library if we were using the tray and not a modal. but now with the extra sort step regardless of what dialog type we are using we always need to use block_place.admin_library. fixed.

16. NOT addressed

I also had started a JS test yesterday but it was not working for some reason. I have put a return statement to skip most of the test and have it pass for now. You may want to just scrap what I have there.

This addresses 2, 12, 14, and 16 from my review.

1. +++ b/core/modules/block_place/block_place.module
   @@ -97,10 +98,15 @@ function block_place_page_attachments(array &$attachments) {
   +    unset($destination_parts['query']['block-place-region-sort']);
   

   Nice! This will save any other parts of the query string and just remove block-place-region-sort

   Maybe in the JS Test which test it with fake-key=fake-value in the query string to make sure that after placing the block and sorting you are sent back to the correct path and qs.

2. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
   @@ -14,20 +13,20 @@
   -   * The entity type manager.
   +   * The block storage.
       *
   -   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   * @var \Drupal\Core\Entity\EntityStorageInterface
   

   +1 better to rely on the storage if that is all we need.

1. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +79,82 @@ function block_place_toolbar() {
   +        // This JS library only needs to be attached here so that the new
   +        // contextual link works with Ajax. Remove
   +        // in https://www.drupal.org/node/2764931.
   

   Nit: missing @todo.

2. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +79,82 @@ function block_place_toolbar() {
   +  $query = \Drupal::request()->query;
   +  if ($region_sort = $query->get('block-place-region-sort')) {
   +    $attachments['#attached']['library'][] = 'block_place/drupal.block_place.js';
   

   Missing cache context.

   $attachments['#cache']['contexts'] = 'url.query_args:block-place-region-sort';
   

3. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +79,82 @@ function block_place_toolbar() {
   +    // Remove block-place-region-sort from the query string.
   +    $destination_parts = UrlHelper::parse(\Drupal::destination()->get());
   +    unset($destination_parts['query']['block-place-region-sort']);
   +    $destination = $destination_parts['path'] . ($destination_parts['query'] ? ('?' . UrlHelper::buildQuery($destination_parts['query'])) : '');
   ...
   +    $url = Url::fromRoute('block_place.sort', ['region' => $region_sort, 'theme' => $theme->getName()], ['query' => ['destination' => $destination]]);
   

   Can't this just be replaced by
   ['query' => \Drupal::destination()->getAsArray()]?

   (Destinations can't have query strings themselves AFAICT?)

4. +++ b/core/modules/block_place/block_place.module
   @@ -77,8 +79,82 @@ function block_place_toolbar() {
   +  // Save a setting to flag pages where block_place mode is active.
   +  $attachments['#attached']['drupalSettings']['block_place']['isInBlockPlaceMode'] = ($region_sort || $query->get('block-place'));
   

   Why is this happening on the server side rather than the client side?

5. +++ b/core/modules/block_place/block_place.routing.yml
   @@ -0,0 +1,15 @@
   +  path: 'admin/structure/block-place/library/{theme}'
   ...
   +  path: 'admin/structure/block-place-sort/{theme}/{region}'
   

   Missing the leading slash.

6. +++ b/core/modules/block_place/block_place.routing.yml
   @@ -0,0 +1,15 @@
   +    _title: 'Order Blocks'
   

   "Blocks" should not be capitalized?

7. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,35 @@
   +    isBlockPlaceMode: () => {
   +      const queryString = decodeURI(window.location.search);
   +      return (/block-place=1/i.test(queryString) || /block-place-region-sort=/i.test(queryString));
   +    },
   

   Remember how I said something belonged on the client side? Well, apparently that's already the case :)

   That means that previously cited code is safe to delete.

8. +++ b/core/modules/block_place/js/block_place.es6.js
   @@ -0,0 +1,35 @@
   +      // If drupalSettings.block_place is set open open dialog.
   

   "open open"

9. +++ b/core/modules/block_place/src/Controller/PlaceBlockLibraryController.php
   @@ -0,0 +1,44 @@
   +    // Alter all 'Place Block' links to use the Offcanvas tray.
   

   "Offcanvas"

10. +++ b/core/modules/block_place/src/Controller/PlaceBlockLibraryController.php
    @@ -0,0 +1,44 @@
    +            'width' => 425,
    

    First time I see a width being specified? Seems inconsistent. Especially if we only do this if outside_in is installed. Oh and actually, it won't have any effect, since the width of the Settings Tray is pre-determined?

11. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,112 @@
    + * Provides a block for sorting block within a region.
    

    "sorting block"

12. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,112 @@
    +class BlockRegionSorterForm extends FormBase {
    ...
    +    drupal_set_message('The block order has been saved.');
    

    "block region sorter"
    vs
    "block order"

    Inconsistent.

13. +++ b/core/modules/block_place/src/Form/BlockRegionSorterForm.php
    @@ -0,0 +1,112 @@
    +      $block->setWeight($weight);
    +      $block->save();
    

    Can be simplified to $block->setWeight($weight)->save();

14. +++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
    @@ -96,24 +121,49 @@ public function build() {
    +            'width' => 350,
    ...
    +            'width' => 700,
    

    Even more widths. Confusing.

15. +++ b/core/modules/block_place/tests/src/FunctionalJavascript/BlockPlaceTest.php
    @@ -0,0 +1,65 @@
    +class BlockPlaceTest extends JavascriptTestBase {
    

    YAY!

16. +++ b/core/modules/block_place/tests/src/FunctionalJavascript/BlockPlaceTest.php
    @@ -0,0 +1,65 @@
    +    // @todo Finish test.
    

    :)

17. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -116,7 +116,9 @@ function setEditModeState(editMode) {
    +        if (!drupalSettings.hasOwnProperty('block_place') || !drupalSettings.block_place.isInBlockPlaceMode) {
    +          setEditModeState(true);
    +        }
    

    Why are we making outside_in consume place_block's JS settings? Why aren't we letting place_block's JS trigger this?

@tim.plunkett's patch in #65 no longer applied to 8.5.x. I haven't addressed any of the other issues. Reroll Only!

+++ b/core/modules/block_place/js/block_place.es6.js
@@ -0,0 +1,35 @@
+  Drupal.blockPlace = {
+    isBlockPlaceMode: () => {
+      const queryString = decodeURI(window.location.search);
+      return (/block-place=1/i.test(queryString) || /block-place-region-sort=/i.test(queryString));
+    },
+  };

Can probably be something like,

Drupal.blockPlace = {
  isBlockPlaceMode() {
    return (/(block-place=1)|(block-place-region-sort=)/i).test(decodeURI(window.location.search));
  },
};

working ...

my changes
@Wim Leers
#67
1. done
2. done
3. done (changed)
4. pending
5. done
6. done
7. pending like 4 is pending(related #70)
8. done
9. this is Offcanvas or Off-canvas, pending
10. pending
11. done,. I remove a word : "block".
12. done
13. done
14. pending
15.
16.
17. pending

Closing as outdated as the block_place module appears to have been moved out of core and into contrib https://www.drupal.org/project/block_place