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
- Follow the pattern in
contextual.module
. - In fact, build upon what
contextual_toolbar()
does — it already is showing up conditionally, we can just alter it conditionally (if the user has theadminister blocks
permission - No more need for
OutsideInManager
norOutsideInCacheContext
!
Remaining tasks
Signed off by @tedbow & @tim.plunkett in #14 + #15.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#9 | 2784853-9.patch | 9.34 KB | Wim Leers |
Comments
Comment #2
xjmComment #3
tedbowComment #4
pixelmord CreditAttribution: pixelmord at Wunder for Hubert Burda Media commentedI 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
Comment #6
Wim LeersYes, 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.Comment #7
Wim LeersIS was not appropriately updated in #3.
Comment #8
Wim LeersAdded formatting to IS to improve clarity, and fix small accuracy problems.
Comment #9
Wim LeersI 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.I think this can be vastly simplified:
outside_in.block_configure
contextual link is present, i.e. when contextual links are present and accessible$toolbar['contextual']
exists, then contextual links are accessible, so we just need to then also check if the user has theadminister blocks
permissionComment #10
Wim LeersClosed #2894588: Add unit test for OutsideInCacheContext.
This also helps #2894584: Settings Tray provides functionality, not an API: mark PHP and JS as internal a lot.
Comment #12
Wim Leers#9 only applies to 8.4.x.
Comment #13
Wim LeersComment #14
tedbowThis 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.
Comment #15
tim.plunkett+1 for the patch.
Still has "needs" tags though
Comment #16
Wim LeersComment #18
webchickI 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!
Comment #19
Wim LeersWell, 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!
Comment #21
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)