Problem/Motivation

The Outside In prototype introduced in #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode allows a user to configure page elements in context. Something very frustating happens when trying to edit the page title.

This is partly an existing usability issue with the page title block (the title and display title make no sense for that block), but it's made much worse with the introduction of a module that lets me click on those words and get a sidebar to do everything but change those words. There are several problems:

  1. The "Display title" checkbox makes no sense for this block.
  2. The "Display title" checkbox does not display the thing in the title box for this block.
  3. The contents of the title box are ignored.
  4. When I click on this, I don't actually want to configure this block at all. I want to edit the node title!

Proposed resolution

TBD

We might even just want to disable Outside In for this block until we can present the user with content Quick Edit by clicking on it as well.

Remaining tasks

Needs design, usability review, implementation, etc.

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm’s picture

Issue summary: View changes
tedbow’s picture

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

Taking another look at this in the current state of the module I think I have a simple solution. The user who wants to edit the title should have a contextual lin k from the page title block "edit title".

We have another issue to change the 'quick edit' on the block to 'open settings tray' so that would give us three links in the page title block dropdown:

  • Configure block
  • Open settings tray
  • Edit title

Configure block will go to the backend form, Open settings tray will show the settings for the block in-place (in this case the block title name which is still confusing...), and edit title should trigger quick edit mode with focus on the title field.

This removes the most annoying WTF of this experience which is that I need to go to the third contextual link down to edit the title of the node. 'Configure block' and 'Open settings tray' are still not crystal clear but at least with the third option the user can do some trial and error and get to where they need to be.

tkoleary’s picture

Issue tags: +Usability
tedbow’s picture

I think once #2782915: Standardize the behavior of links when Outside In editing mode is enabled lands(hopefully soon) this problem will be easier with better UX.

With that issue in "Edit Mode" if you click an area that is covered by QuickEdit then you the quick edit toolbar will be invoked.
We could extend this functionality to the Title block if it for a QuickEdit entity. The title in this block has the attribute data-quickedit-field-id if it is for a QuickEdit entity.

Then I think we should remove the Settings Tray functionality altogether for this block. It is a form without any real functionality. It has 2 elements. A checkbox for "Display title" that in all other blocks shows the title of the block. If you click it shows the page title twice. The textfield has even less functionality. It literally has no functionality. It updates the title of the block but because the way the checkbox behaves the textfield changes will never show on the site.

You would still be able to get the advanced block form where you could delete the block or changes its region if needed.

tedbow’s picture

Thought a little more about it and no reason to wait for #2782915: Standardize the behavior of links when Outside In editing mode is enabled

Removing the page title block from the "Edit mode" makes sense regardless. Adding the trigger for QuickEdit could be a follow up issue.

This page remove the page title block from the Setting Tray module functionality.

tedbow’s picture

Status: Active » Needs review

Setting needs review.

Also couple question about my patch

  1. +++ b/core/modules/outside_in/outside_in.module
    @@ -55,6 +60,12 @@ function outside_in_block_view_alter(array &$build) {
    +    // If this block should not be editable flag for contextual link removable.
    +    // The individual links are not available here to remove.
    +    $build['#contextual_links']['block']['metadata']['remove_outside_in'] = TRUE;
    

    I could find a better to flag that the block should not have the contextual link. In hook_contextual_links_view_alter we don't have plugin_id any more. We could check from the block id = "*_page_title" but that seems hacky and nothing would stop someone from using that block id pattern for another block.

  2. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,15 @@
    +  public function isBlockEditable($plugin_id);
    

    Wasn't sure if this belonged in the interface but fits the description "Provides an interface for managing information related to Outside-In."

tkoleary’s picture

Tested in simplytest.me. Passes usability review.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/outside_in/outside_in.module
    @@ -34,6 +34,11 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
    +    if (!empty($element['#contextual_links']['block']['metadata']['remove_outside_in'])) {
    +      unset($element['#links']['outside-inblock-configure']);
    +      unset($items['outside_in.block_configure']);
    +      return;
    +    }
    
    @@ -55,6 +60,12 @@ function outside_in_block_view_alter(array &$build) {
    +  if (!\Drupal::service('outside_in.manager')->isBlockEditable($build['#plugin_id']) && isset($build['#contextual_links']['block'])) {
    +    // If this block should not be editable flag for contextual link removable.
    +    // The individual links are not available here to remove.
    +    $build['#contextual_links']['block']['metadata']['remove_outside_in'] = TRUE;
    +  }
    

    This needs some documentation. I needed to re-read this 5 times to figure out what's going on.

    The second hunk sets a "fake" bit of contextual link metadata.

    The first hunk detects the presence of that, and if so, unsets a particular contextual link.

  2. +++ b/core/modules/outside_in/src/OutsideInManager.php
    @@ -63,4 +63,12 @@ public function isApplicable() {
    +    $non_editable_blocks = ['page_title_block', 'system_main_block'];
    

    Nit: $readonly_blocks probably makes more sense?

  3. +++ b/core/modules/outside_in/src/OutsideInManager.php
    @@ -63,4 +63,12 @@ public function isApplicable() {
    +    return !in_array($plugin_id, $non_editable_blocks);
    

    Let's make this strict (in_array(…, …, TRUE)

  4. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,15 @@
    +   * Checks whether a block should be covered by this module.
    ...
    +  public function isBlockEditable($plugin_id);
    

    Nit: comment does not match function name.

  5. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,15 @@
    +   *   The plugin id for the block to check.
    

    Nit: s/id/ID/

  6. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,15 @@
    +   *   TRUE if the block should the "Quick Edit" link provided by this module.
    

    Incomplete sentence.

The reason this is so surprisingly complex: the Contextual Links API was never designed for conditional contextual links. The Quick Edit module generates contextual links in JS (on the client side), so it can do this conditionally quite easily. This patch opts to do it on the server side. Hence it's fairly convoluted.

The approach in this patch can work. I think it's okay… but I can't help but wonder whether it wouldn't be a whole lot simpler to do this on the client side instead, much like Quick Edit.

tedbow’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
4.04 KB
3.99 KB

@Wim Leers thanks for the review.

Yes I think you right it is complicated to do the removal on the server side.

This patch just sets an attribute if the link should be removed. Then the actual removal is done via Javascript.

From the review
1. Mostly removed now. I add @see comment to point the .js file and to the the module file. Hopefully clearer what is going on.
2. I like "non_editable" because it directly says what the user is restricted by.
3. fixed
4. fixed
5. fixed

Wim Leers’s picture

  1. +++ b/core/modules/outside_in/outside_in.module
    @@ -55,6 +55,14 @@ function outside_in_block_view_alter(array &$build) {
    +    // to remove the link on the client-side.
    

    s/client-side/client side/

  2. +++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
    @@ -15,4 +15,16 @@
    +  public function isBlockEditable($plugin_id);
    

    Are you sure you want to add a block-specific method to the interface? Outside-In is not restricted to blocks AFAIK?

    Related: I'd strongly recommend doing what BigPipe did here too: #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal + #2835758: Remove BigPipeInterface and move all of its docs to the implementation.

tedbow’s picture

re #14
1. fixed
2. Removed this from the interface and moved to a function in the module file.

On the related note I think that makes sense and will make follow up issues.

tedbow’s picture

Ok adding test for the contextual link for the title block being excluded.

Also attaching a TEST_ONLY patch.

The last submitted patch, 16: 2782891-16-TEST_ONLY.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/outside_in/outside_in.module
@@ -154,3 +164,18 @@ function outside_in_css_alter(&$css, AttachedAssetsInterface $assets) {
+  $non_editable_blocks = ['page_title_block', 'system_main_block'];

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -364,4 +364,16 @@ protected function pressToolbarEditButton() {
+   * Tests that title block does not have a outside_in contextual link.

Shouldn't we also test the system_main_block block?

Other than that, RTBC.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
4.91 KB

@Wim Leers thanks for the review!

Yes we should check the content block also. I also realized that not only should we be checking for the contextual link being excluded we should also make sure that the data-drupal-outsidein attribute is not added which would mark the the block as "editable".

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -368,12 +368,23 @@ protected function pressToolbarEditButton() {
+    // We do not need to place the main content block.
+    // It's ID will just be 'content'.

Well, that's just the fallback behavior in \Drupal\block\Plugin\DisplayVariant\BlockPageVariant::build(). In that case, it's not even an actual block.

So I'm not sure that this patch is correct. I think we should place the block. And we should have a separate test ensuring that you also don't get outside in functionality in case no "main content" block is placed.

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
5.11 KB

So I'm not sure that this patch is correct. I think we should place the block.

Ok now placing both the blocks that should be excluded in the same way.

And we should have a separate test ensuring that you also don't get outside in functionality in case no "main content" block is placed.

I am not sure we need this \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks tests Settings Tray edit mode functionality with explicitly placing the main content block.

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs re-roll because of #2862625: Rename offcanvas to two words in code and comments. and other recent commits

RajeevK’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.18 KB

Re-rolling patch for test & review after rebase.

tedbow’s picture

@RajeevK reroll looks great, Thanks!!!

Leaving as needs review because changes in #21(rerolled in #23 still need be reviewed.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/outside_in/outside_in.module
@@ -163,3 +173,18 @@ function outside_in_css_alter(&$css, AttachedAssetsInterface $assets) {
+function _outside_in_is_block_editable($plugin_id) {

Perhaps add an explicit @internal?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: 2782891-23.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

Re-rolled #23 plus Wims' idea in #25