Postponed by:

Problem/Motivation

When block place module is enabled and outside in edit mode is turned on if the user clicks "Place block", the configuration form appears in a modal. If, after placing the block the user clicks the block the same configuration form appears in the off-canvas tray.

Proposed resolution

When both outside in and block place are enabled use the off-canvas tray for the place block form.

Remaining tasks

  1. Javascript tests for sorting functionality
  2. When placing a block in region with no other blocks don't open the tray to sort blocks(there will be only 1)
  3. Is there better solution for than block_place_page_top use?
  4. Ensure that when "Place Block" mode is entered Settings Tray "Edit Mode" is exited.(probably need changes to Settings Tray)

User interface changes

Place blocks form appears in the tray, not a modal.

API changes

Data model changes

None

Files: 
CommentFileSizeAuthor
#51 interdiff.txt1.45 KBnerdstein
#51 2784495-51.patch22 KBnerdstein
#50 normalize_block_place-2784495-50.patch22.05 KBdroplet
#50 interdiff.patch1.71 KBdroplet
#49 2784495-49.patch22.04 KBtedbow
#49 interdiff-40-49.txt1.83 KBtedbow
#40 2784495-40.patch22.08 KBtedbow
#40 interdiff-2784495-38-40.txt4.18 KBtedbow
#38 interdiff-35-38.txt1.41 KBtedbow
#38 2784495-38.patch20.22 KBtedbow
#35 2784495-35.patch20.18 KBtedbow
#35 interdiff-32-34.txt3.43 KBtedbow
#32 2784495-32.patch19.58 KBtedbow
#32 interdiff-29-32.txt9.59 KBtedbow
#29 interdiff-21-29.txt9.69 KBtedbow
#29 2784495-29.patch14.37 KBtedbow
#21 2784495-21.patch12.88 KBtedbow
#21 interdiff-19-21.txt932 bytestedbow
#19 2784495-19.patch11.96 KBtedbow
#19 interdiff-9-19.txt8.08 KBtedbow
#9 2784495-9.patch6.56 KBtedbow
#7 2784495-block_place_changes-do-not-test.patch6.37 KBtedbow
#7 2784495-including_2786459-7.patch39.59 KBtedbow
#6 2784443-block_place_offcanvas-6.patch5.71 KBtedbow

Comments

tkoleary created an issue. See original summary.

xjm’s picture

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

xjm’s picture

Issue tags: -quickedit, -modal, -jQuery, -Ajax
webchick’s picture

Issue tags: +sprint
tedbow’s picture

Component: quickedit.module » outside_in.module
tedbow’s picture

Status: Active » Needs review
Issue tags: -Outside-in
FileSize
5.71 KB

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?

tedbow’s picture

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’s picture

@tedbow

Can't get this patch to run in simplytest.me

tedbow’s picture

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

tedbow’s picture

Title: Normalize block place and outside-in experiences » [PP-1] Normalize block place and outside-in experiences
Status: Needs review » Postponed
Related issues: +#2804639: Offcanvas dialog width not respected

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

This patch needs to be able to set the width.

tedbow’s picture

Status: Postponed » Needs review
Issue tags: -sprint

#2804639: Offcanvas dialog width not respected was fixed!

Current patch still applies

tedbow’s picture

Title: [PP-1] Normalize block place and outside-in experiences » Normalize block place and outside-in experiences
tkoleary’s picture

@tedbow

Patch works great. Adding a task to highlight the region the block is being placed int like we do when a block is selected. https://www.drupal.org/node/2825413

tkoleary’s picture

Issue tags: +sprint
tkoleary’s picture

Issue tags: +JavaScript

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

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.

Wim Leers’s picture

Status: Needs review » Needs work

Per #17.

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.08 KB
11.96 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: 2784495-19.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
932 bytes
12.88 KB

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

SKAUGHT’s picture

Status: Needs review » Needs work

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.

tedbow’s picture

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.

tedbow’s picture

Component: outside_in.module » block_place.module
nerdstein’s picture

Issue summary: View changes
Status: Needs work » Postponed

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.

nerdstein’s picture

Issue summary: View changes
nerdstein’s picture

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).

nerdstein’s picture

This patch is a significant step forward to finish the user experience proposed in #2739075: [plan] Graduate the Place Blocks module to a "real" core module.

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.

tedbow’s picture

Status: Postponed » Needs review
FileSize
14.37 KB
9.69 KB

@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

nerdstein’s picture

Status: Needs review » Reviewed & tested by the community

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] Graduate the Place Blocks module to a "real" core module.

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.

tedbow’s picture

Status: Reviewed & tested by the community » Needs work

@nerdstein thanks for the review again.

I left this comment about sorting on #2739075: [plan] Graduate the Place Blocks module to a "real" core module 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?
tedbow’s picture

Status: Needs work » Needs review
FileSize
9.59 KB
19.58 KB

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.

Status: Needs review » Needs work

The last submitted patch, 32: 2784495-32.patch, failed testing. View results

tedbow’s picture

Issue summary: View changes

Added remaining tasks to summary

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
20.18 KB

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.

Status: Needs review » Needs work

The last submitted patch, 35: 2784495-35.patch, failed testing. View results

SKAUGHT’s picture

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
20.22 KB
1.41 KB

Fixes to JS standards

Status: Needs review » Needs work

The last submitted patch, 38: 2784495-38.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.18 KB
22.08 KB

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

Status: Needs review » Needs work

The last submitted patch, 40: 2784495-40.patch, failed testing. View results

SKAUGHT’s picture

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.

nerdstein’s picture

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.

nerdstein’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 40: 2784495-40.patch, failed testing. View results

nerdstein’s picture

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.

nerdstein’s picture

Version: 8.3.x-dev » 8.4.x-dev

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.

droplet’s picture

I expected ES6 ready in all new JS files :)

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
22.04 KB

@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.

droplet’s picture

@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)

nerdstein’s picture

@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.

nerdstein’s picture

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?