Problem/Motivation

\Drupal\outside_in\OutsideInManager::isApplicable() currently tries to determine when the Outside In is applicable on a request. We are currently now showing if it is an admin route. If we are going to keep the current logic we should probably check if the admin theme is actually different from the default theme.

The Outside In module is tied the functionality of the "Edit" item in the toolbar that contextual module creates. This item is always added to the toolbar but is hidden if no contextual links are on the page. The seven theme removes all contextual links via seven_preprocess_block(). So this why the Edit toolbar tab doesn't show in the toolbar. The drupal.contextual-links library is loaded on all pages whether or not links are there. Should we follow this pattern?

Proposed resolution

  1. Follow the pattern in contextual.module.
  2. In fact, build upon what contextual_toolbar() does — it already is showing up conditionally, we can just alter it conditionally (if the user has the administer blocks permission
  3. No more need for OutsideInManager nor OutsideInCacheContext!

Remaining tasks

Signed off by @tedbow & @tim.plunkett in #14 + #15.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

xjm’s picture

Issue tags: +Outside-in
tedbow’s picture

Component: asset library system » outside_in.module
pixelmord’s picture

I think the concept of outside-in and the pattern of the off-canvas tray totally make sense also for admin routes. I discussed that also during one of the last Drupal UX Hangouts and we came to the conclusion that it would be a good idea to open up the concept to applications that can be in the admin interface.

Imagine for example to use that pattern for the media library for selecting media for a media field. You can have both the form and the library side by side.

In the light of that I would say that it would not be good to restrict the loading of the library to non-admin routes

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.

Wim Leers’s picture

Issue tags: -Outside-in

Yes, follow the same pattern as contextual. Then we have both related/similar modules using the same pattern. Easier to maintain. And if it's wrong, we can update both in tandem.

Wim Leers’s picture

Issue summary: View changes

IS was not appropriately updated in #3.

Wim Leers’s picture

Issue summary: View changes

Added formatting to IS to improve clarity, and fix small accuracy problems.

Wim Leers’s picture

I started working on #2894588: Add unit test for OutsideInCacheContext, then I realized OutsideInManager doesn't really make sense… it was added in the original MVP issue — #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode, and was never revisited or reviewed.

  1. Its current implementation varies by "admin route or not", minus the block admin demo one, but this issue is suggesting it should be changed to vary by theme instead.
  2. Its current implementation varies by "'administer blocks' permission or not", for which #2822965: Make Settings tray work with other things besides "Quick Edit" on blocks apparently also exists.

I think this can be vastly simplified:

  1. we don't need the "admin route or not" check — we simply need this whenever the outside_in.block_configure contextual link is present, i.e. when contextual links are present and accessible
  2. if $toolbar['contextual'] exists, then contextual links are accessible, so we just need to then also check if the user has the administer blocks permission

Status: Needs review » Needs work

The last submitted patch, 9: 2784853-9.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review

#9 only applies to 8.4.x.

Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks good to me. Chatting with @Wim Leers a bit about the reasoning and it boils down to "Settings Tray should work wherever Contextual works".
Seven and other admin themes don't allow contextual links but other admin themes might allow theme. So in those cases if the admin theme wants to support contextual links then there is no reason Settings shouldn't also work.

tim.plunkett’s picture

+1 for the patch.
Still has "needs" tags though

Wim Leers’s picture

Title: Determine when Outside In library should be loaded » Determine when Outside In library should be loaded: piggyback on contextual_toolbar()
Issue summary: View changes
Issue tags: -Needs issue summary update, -Needs title update

  • webchick committed ee9dacf on 8.4.x
    Issue #2784853 by Wim Leers, tedbow: Determine when Outside In library...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

I like this idea a lot; far less code to maintain, and one central "toggle" for "do admin-y things from here." Awesome sauce. Hopefully we can explore @pixelmord's idea about opening this up to admin routes as well in another issue.

Committed ee9dacf and pushed to 8.4.x. Thanks!

Wim Leers’s picture

Well, it's the Seven theme that removes Contextual Links. Settings Tray builds on Contextual Links (it progressively enhances it if you will). There's nothing preventing Contextual Links and therefore Settings Tray from working in the admin theme.

And YAY!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)