Problem/Motivation
The parent issue #2762505: Introduce "outside in" quick edit pattern to core
has a laid out a broad scope. This issue is for the tackling items in that issue that are marked as [required], "required for initial commit as experimental module"
Please read the parent issue to familiarize yourself with the broader issue.
Remember more can be added after we get the initial commit.
Please read the handbook page on Experimental Modules if you aren't familiar with this new process.
If you are testing this issue you may need to start a new browser session or login/logout to get the patch to work. This is known issue that we are working on.
Proposed resolution
Implement Required items only from #2762505: Introduce "outside in" quick edit pattern to core.
Please read the parent issue. If you would like to comment/debate the outside in pattern itself please do so on that issue not this one.
Also for clarity this issue is not to debate what is required for the initial version of the Outside In module. If there are additional required, must have, should have, could have items lets discuss on the parent issue.
The parent issue also contains the "final" target designs. For now, this is what the patch looks like visually (to be tidied up in must-have follow-up #2781577: Properly style outside-in off canvas tray)
Not in scope
- Re-designing Quick Edit for content entities
- Re-designing the Place Blocks feature (that's #2739079: Create a validated design for Block Place module)
- Designing for use cases outside of ones specified here
- Other types of trays that pop up from other
- Per our new agile core development process, Information Architecture/Nomenclature and other changes to spec itself (until it's in, then open season again!)
Remaining tasks
This is a list of Required items from #2762505: Introduce "outside in" quick edit pattern to core. Let's get committed and then we can make even more awesome. Remember it's Experimental! If we get this committed by the Week of Aug 3rd this can get as experimental for 8.2.
Move Quick Edit's "Edit" link to the far left of the ToolbarWhen clicked, triggers Edit modeAdd "Quick Edit" to all contextual links that are editable; also triggers edit modeWhen clicked, sidebar opens, showing the target's configuration- Except nodes, which trigger the existing "quick edit" functionality without opening sidebar
Implement Edit button as View/Edit mode toggleFlip to secondary Toolbar with white backgroundStyling could be improved - Edit icon disappears.Disable clickability of all menu / content links on page- On mobile, outline all clickable regions on page - DELAYED: The existing edit mode seems to be broken for mobile in 8.2.x
Trigger sidebar from clicking/tapping anywhere within a dotted regionOpen simplified Block form in side-bar. @see #2763157: Allow plugins to provide multiple formsProvide each simplified form in the tray with an “advanced” link that loads the full version of the form in the admin theme.
Known issues
simplified Block form: doesn't save values"Quick Edit" contextual links on existing blocks might not show up until you edit blocks or change permissions. This is because of client side caching from the Contextual module. #2773591: New contextual links are not available after a module is installed
Blocking issues
#2764931: Contextual links don't work with 'use-ajax' links - will stop "Add "Quick Edit" to all contextual links"There is temporary fix in this issue but should be fixed in contextual module#2763157: Allow plugins to provide multiple forms - will stop Open simplified Block form in side-bar - but this will be a very simple change to this issue once it is finished.We have put this logic into this module but should be moved out into core if this module is made non-experimental.
User interface changes
There will be a tray that slides in from the right, off-canvas that modules can use to present configuration forms in-place.
API changes
No core API changes
Will provide a new Ajax dialog type that can be invoked like this:
$element['attributes'] = [
'class' => ['use-ajax'],
'data-dialog-type' => 'offcanvas',
];
Comment | File | Size | Author |
---|---|---|---|
#235 | outside-in-1-clip.gif | 1004.85 KB | tkoleary |
#224 | interdiff.txt | 5.55 KB | effulgentsia |
#224 | 2753941-outside_in-224.patch | 66.97 KB | effulgentsia |
#222 | interdiff-2753941-218-222.txt | 9.33 KB | tedbow |
#222 | 2753941-222.patch | 66.91 KB | tedbow |
Comments
Comment #2
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #3
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #4
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #5
miro_dietikerThis looks nice. Happy we can figure out how to go more far into this direction.
For Paragraphs we also want some nice frontend like "Add" workflow or exposure of additional configuration that should not clutter the content editing form...
In Drupal we decided to display the menu (left) and also the toolbar proposal right that changes the content width.
This triggers a variable width for the main area, and requires very complex CSS variations (different break points varying .toolbar-tray-open).
To avoid this problem, all other CMS'es and libraries display the menu on top of the document, making the document width / wrapping stable and independent of the menu/try visiblity conditions.
Finally, the example shown can be avoided anyway:
I usually argue agains modal, but an "Add" action dialog does not need to be displayed off main area. It can easily be modal.
Comment #6
tkoleary CreditAttribution: tkoleary at Acquia commentedI'm not sure I follow your logic here. The vertical toolbar as it exists now in Drupal 8 uses negative margin to essentially "trick" the site page into behaving as if the edge of the menu is the edge of the viewport. The off-canvas tray would use the same technique only on the righthand side (LTR). A responsive site should already be designed to respond to changes in viewport width to accommodate any number of sizes so a vertical tray would not place any new burden on front-end themes that does not already exist.
This is simply not true. Many CMSs use vertical navigation as well as vertical trays or drawers for configuration, for example Squarespace, Adobe Experience Manager, Wordpress, and Neos just to name a few. It's not that I'm opposed to horizontal trays either on the top or the bottom, in fact I think that the solution should encompass both, which is why I added #2753949: [PP-1] Add a "bottom tray" to off-canvas configuration. Vertical and horizontal trays each have advantages for different types of configuration and we need them both.
The advantage of putting "Add" in the tray and not using a modal is that the user does not lose context and can "try out" the block, ie. place it and immediately see it in place, which removes the "back and forth" that a modal imposes.
Comment #7
SKAUGHTJust to add some thoughts:
One issue not only for off side menus is the similar to mega menus. Which is how to provide a functional menu with degraded javascript. Often down with a combination of JS on top, with css hide/show for hover events. which also get into where that html is rendered -- usually just after the
<body>
tag or in a block with a bunch of JS to move the html, for example.The difficulty with bottom trays (more in web than app) is determining where the bottom is and change in orientation, etc. This is assuming the browser does support viewport -- now a days, most do.
Certainly lots of other system use all sorts of tools. Keep in mind how well those tools work in which browser. Often enough they have large support limitations, and more often very flawed functionality as that result.
I might say that's why drupal's toolbar isn't off-canvus. To provide the widest level of supportability: keep it simple, keep it on top.
Modal's would probably be better form mobile screen sizes. as off-canvus lists get to be unusable depending on amount of content in them. especially when in this 'add context'
Comment #8
webchickComment #9
Bojhan CreditAttribution: Bojhan as a volunteer commented@Kevin/Ted Can you update the summary it does not reflect the scoping in the meta issue.
Comment #10
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
Interesting points.
I agree about menus and particularly mega-menus. In this case though, the primary use of the off canvas tray is not menus but actual configuration, in fact the only menus displayed will be when you are actually editing the menu links, not using them to navigate.
In mobile both modals and off-canvas trays suffer the same fate of internal scroll if there is a long list. That's why we have the ability to filter the list.
As far as
Comment #11
tedbowOk here is a patch that introduces the new experimental module Outside In(ta-da!).
Thanks to Matt Grill(drpal) for the JS and CSS in this patch.
The patch currently just provides a way to designate certain links open up in the sidebar.
Currently it adds to ajax dialog system to add a new dialog type "offcanvas".
So you would make a link open in the off-canvas tray like this:
The code and the screenshot are using the test module Offcanvas tests which is included in the new module. We will need to write JS tests using this module for the OffCanvas functionality.
If you want test the functionality you will current need to enable Offcanvas tests(offcanvas_test).
Know problems
Comment #12
GrandmaGlassesRopeManThis should fix the animation issues with the patch from #11.
Comment #13
GrandmaGlassesRopeManThe interdiff between #11 and #12.
Comment #16
tedbowOk this patch
Regarding the "Quick Edit" contextual links. Current problems:
Comment #18
tedbowComment #19
tedbowFixes for composer.json, hook_help, OffCanvasDialogTest
Addressing test fails of #16
Comment #20
tedbowComment #22
GrandmaGlassesRopeManHere are some changes that adjust how width transition is handled. This fixes an issue when zooming in our out and also moves away from fixed pixel widths in favor of percentages.
Comment #23
SKAUGHTTrigger tests
Comment #24
SKAUGHTtests pass.
Core Themes.
Bartik: CSS positioning needs work. it seems to be jumping below the height of the used page.
stark: fails.
Contrib Themes.
passed through: Adminimal, Danland & Zircon
all fail.
the JS is too dependant on expected css classes that bartik has. this must be done much more generically.
Perhaps (preproccess) adding real div with class to html.html.twig preceeding {{page}} so that that wrapping will always be known. could add to system module twig directly, but as an experimental module it's then outside this module.
Comment #25
SKAUGHTadd preprocess page_top and page_bottom to wrap it all (all {{page}} render in all themes)
note: the canvas menu mis-align (image #24) is constant in other themes as well.
also: I had left in 2 use clauses.. they may not be needed... (sorry, sunday experimenting)
Comment #27
SKAUGHTre-testing. i think it's a CI issue.
other review note: if 'Click me 1' is open, and you click 'Click Me 2' -- content does not update.
also:
desktop: window resize needs to be handled.
mobile: orientation.
Comment #28
SKAUGHTComment #29
SKAUGHTComment #30
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI tried out patch in #25 for accessibility, to get an early feel for what was going on here. Here are my initial thoughts, with a patch.
<button>
element so it can be operated by keyboard.aria-label
for now, but that isn't the only option. I guess the 'x' text will eventually be replaced with a nicer image icon?<h1>
helps and is probably an appropriate heading level. As for what semantic to use for the tray container element, that's a tricky one. For now, I've usedrole="region"
and associated the<h1>
as the accessible label.The tricky part I think, is what role to use for the tray container element. It's tempting to describe it as a (non-modal)
role="dialog"
, however that would entail certain expected keyboard behaviours, so it's just arole="region"
for now.Comment #31
azuracatprince CreditAttribution: azuracatprince commentedThis is a really nice idea. It gives an interesting opportunity to even render random modals as offcanvas always.
Sorry for this nitpick, but if we want to get a lot of feedback from people, I believe in making it sort of usable outside of bartik is a good idea.
id="bartik"
is just available from within Bartik at the moment.Just a random suggestion: I've heard we have actual javascript supported testing now, so what about actually testing this feature using it? Testing a javascript only feature by using PHP only, feels sort of like cheating on yourself. Sure, other parts of core might do something else, but people considered creationism as the right way of thinking once as well, and the world moved along in the meantime.
Comment #32
SKAUGHT#31
JavaScript test: yes, new to core. I'm sure that will happen sooner than later.
I'll check over later for any other specific bartik refs -- they should not any for sure
I think we really need a cleaner module namespace/variable/class names. And should be addresses soon than later. So: what is a better way to scope this: "offcanvas" "canvas_tray"?
Are we only making this for right side. Shouldn't we be prepping ANY of the four sides? #2753949: [PP-1] Add a "bottom tray" to off-canvas configuration shouldn't be separate
Also, rtf language: JS should swap sides with Lang switch
Comment #33
tedbow@SKAUGHT this is going to be part of the larger experimental module described here #2762505: Introduce "outside in" quick edit pattern to core. So changing the variable name might work I don't think we want to change the module name. We are trying to 1 experimental module in that will include the off canvas tray. We might want to change offcanvas to off_canvas or off_canvas_tray but I don't think we want to loss the "off" as this seems to use describe this pattern in other parts of web development.
Since we trying to get this in for an experimental module we are free to add these after the experimental module is committed. I think we should focus just on right-side for now as the MVP for the module. We could later use data-dialog-options to pass in other options such side of the tray. Looking at the requirements for experimental modules I think this is something that could easily be tackled in follow ups.
Hmmm. Good point. Do you know if the "Manage" navigation switches sides in rtl languages in Drupal core? If it does I think we should switch side. Could this be something we tackle in follow ups too?
Comment #34
tedbow@azuracatprince
Yes good point we will be adding JS tests. The current tests were just easy to add because they are the same as existing core test \Drupal\system\Tests\Ajax\DialogTest.
We can either take them out or leave them in after we add JS tests. But I think we should leave them in for now they at least add some test coverage for patches added to this issue.
Comment #35
tedbow@SKAUGHT, I checked and switching to a RTL language does switch the manage menu position. So we should switch too.
Comment #36
SKAUGHT#33.
okay. I get wanting to make a MVP before adding bottom/top and right/left setting. Of course, lang appropriate positioning will to provide this tray mechanism alone.
Then, what is next steps. Summary currently clearly shows are use with Place Blocks and the other toolbar reworking you've proposed. Certainly we need a new block that contains the available blocks and related regions trigger hrefs. What we seem to be missing is space for the more specific block configurations (ie menu block selects what menu). Or are we leaving that for post placement, where the user would simple use existing context links to alter configs.
Comment #37
tedbowHere is patch that address problem noted in #31 about
Changed this to target #canvas-tray instead of #page.
This seems to solve a height issue that was introduced in #25. The tray now takes up the entire height of the page in Bartik and Stark. But there is still a problem with tray overlapping the page a little bit.
This from Bartik
You can see only the "My Account" link in the top right and not the "Logout Link"
Comment #39
tedbowOk I am changing the parent issue and changing this issue to be the MVP for the Outside In module.
I did this because this is introducing a new module so it would be difficult to actually commit this as a half finished module
Please read #2762505: Introduce "outside in" quick edit pattern to core.
I have added remaining tasks to this issue that are marked Required in the parent.
Comment #40
tedbowAdded section on blocking issues and what they block.
Comment #41
tedbowOk this adds \Drupal\Tests\outside_in\FunctionalJavascript\OffCanvasTest which is Javascript based test simply tests clicking 2 links that will open and close the tray.
I am testing it with Bartik and Stark for now and it working fine locally.
Comment #44
tim.plunkettAdded a couple interfaces, a service, an alter hook, and some tests.
Usage:
Comment #46
tim.plunkettRemoving usage of "sidebar"
Comment #48
Bojhan CreditAttribution: Bojhan as a volunteer commentedAssuming we are not doing menu anymore? As @webchick noted in the other issue?
Comment #49
SKAUGHTfixes float form #24 (seems to been a safari issue)
cleanup doc from previous 'page-wrapper' stuff
fixed close tray animation
replace 'x' close button with hidden text. used /misc close icon background
Comment #51
tkoleary CreditAttribution: tkoleary at Acquia commentedMerged Tims changes with some new CSS.
Hadn't seen Skaughts changes in previous comment. Looks like a merge is needed.
Comment #52
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
Hey, just looked at your interdiff. Many new changes exist in my patch at 51 that would be difficult to merge with your patch at 49. Could you have a look at the one at 50 and see if you can merge to that one?
I also did some preliminary work on the RTL, let me know what you think.
Comment #53
SKAUGHTi'll try to merge.
Comment #54
SKAUGHTIt seems we did do a couple of the very same things.. aside from all the css you've added. let's go ahead with your changes rather than merge. I think there's one little thing i'll add back in later. i need to look over your changes to see where your at more clearly..
cheers.
Comment #55
SKAUGHTGood-morn:
this includes the 2 worth changes from 49.
@tkoleary
i've removed the old offcanvas.css as your libraries changes no longer uses it.
Comment #56
SKAUGHTis CI running today? (:
Comment #58
tim.plunkettIMO this should be prefixed with the module name, and should use underscores and not hyphens.
outside_in_tray_[open|close]
would be fineComment #60
SKAUGHT#58. changed
also fixed height of close button, now that it's visually-hidden text
added in routing to allow new content to be added if tray is already open.
Comment #62
SKAUGHTps: rtl needs more work. myself, i can put some time to that later today/this eve.
Comment #63
tedbowOk, here is patch the updates the JS test to work with the new CSS animations(I assume that is what made them break).
It also makes 1 small change to the CSS to make the close button appear which was also causing the test to fail. I just commented out the line because I don't think the button is on the side we really want. But it does show now and you can close the tray both manually and in the test.
Comment #65
tkoleary CreditAttribution: tkoleary at Acquia commentedThis one resolves some issues with the close button as well as RTL and animation tweaks.
Comment #66
tkoleary CreditAttribution: tkoleary at Acquia commentedRerolled patch
Comment #67
tedbowOk this patch
I think changes in #66 break the current JS test but I am not sure why yet.
Comment #69
SKAUGHTfixes secondary reopening of tray. one() event need to be on pageWrapper not offCanvasWrapper
Comment #70
SKAUGHTComment #72
tedbowFound a bug. If you install Outside In on an existing site then the existing blocks will not the "Quick Edit" contextual links. Placing a new block will work or updating an existing on.
We will probably need to invalidate a cache on hook_install(still the right one?). Maybe 'block_view'?
Also will need a test for this.
Haven't had a chance to look at #69
Comment #73
SKAUGHTCurrently, we have the tray set to 25% of width. Which is fine on desktop and okay on pads for width. but, phones are not usable at 25%.
question to all:
how about adding in a breakpoint js. like:
these are just 2 i've come across quickly, I've not tested to see how good a general fit they are. long term, probably very useful for core to have a JS tool for breakpoints.
Or if anyone has any other preferences/suggestions? or aware of another issue regarding this.
Comment #74
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
I think the plan is to use breakpoint module as a dependency.
Comment #75
SKAUGHTahh.. i'm just been peeking through toolbar module to see how it actually setup. admittedly, i haven't played with breakpoint. i actually assume backbone was handling more there.. i'll have to do some more studying of it.
Comment #76
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
That would be awesome. I am equally unfamiliar with it. Jesse Beach wrote all that code when I first designed toolbar.
Comment #77
GrandmaGlassesRopeManThis adds some features to to allow users to realize they are in edit mode (using localstorage, same as the tool bar) and automatically open the off canvas element for edits when in edit mode.
Comment #78
SKAUGHTretouch annouce() messages
remove return false on prototype
Comment #79
SKAUGHTComment #81
Gábor HojtsyMarking for Usability sprint given activity :)
Comment #82
GrandmaGlassesRopeManThis adds the ability to toggle the focus class on outside-in-editable items when using the edit button.
Comment #83
tedbowThis patches fixes the problem where offcanvas won't work as contextual links.
The problem is really a problem with the contextual module see #2764931: Contextual links don't work with 'use-ajax' links
But we can fix it here to keep the experimental module self contained.
Comment #84
tim.plunkettFixing a bunch of issues throughout, thanks to PHPStorm.
Also adding a route for the offcanvas block form.
Comment #86
SKAUGHT#83 there seems to be a dashed outline bleeding on .contextual-region.focus classes
EDIT: this seem not to be happening now. maybe browser cache and switching patches.
Comment #87
SKAUGHT#84
there is a deletion of some access_test stuff i think may unconnected here
Comment #88
SKAUGHT@tkoleary (maybe)
after opening with 'quick edit' context link (yeppie..working!!)
Comment #89
MixologicMissing @group ?
Thats the first thing I look at when the testrunner aborts like that soon after
php /var/www/html/core/scripts/run-tests.sh --list --php /opt/phpenv/shims/php > /var/www/html/artifacts/testgroups.txt
.Comment #90
tim.plunkettThanks! Seems to have been it.
Also restored the files I accidentally deleted.
Comment #91
SKAUGHTlinking.
wonder how this will effect once in sidebar. hopefully just well.
otherwise to generally note:
Comment #93
tim.plunkettComment #95
tedbowThis patch hide items in the besides "Edit" and "Place Block"(if present).
It also toggles the button from "Edit" to "View" and back.
It also applies the class "js-outsidein-edit-mode" to the toolbar. We can now add css to make the toolbar a different color in edit mode. I didn't want to do the css myself.
Comment #96
SKAUGHTdo we need to have the text change: Edit <—> View. sorry, i think we had a conversation about this from place block creation about the same title change.
also: is this dashed line consistent expected/useful??
Comment #97
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
Two good points but we already had lengthy discussions about both of these issues in weekly ux meetings. You can find recordings of them on G.D.O.
Not that the questions can't be re-opened but that should happen in the UX meetings (please come!) and implementation discussions should happen here.
Comment #98
tkoleary CreditAttribution: tkoleary at Acquia commentedRefactored CSS to rely on Bartik, specific CSS can be a follow-up
Comment #99
tkoleary CreditAttribution: tkoleary at Acquia commentedAnd yes, the interdiff.
Some things that are not CSS are in there because I had to resolve merge conflicts for two previous patches.
If anyone has tips on merging in individual files they would be helpful. Stackoverflow has not.
Comment #100
tedbow@kevin the patch in #98 at least contains additions of new files not within the "Outside In" that weren't included in the 2 latest patch #95 and #93. I think they were from previous patches but removed.
I think it also has some other changes that should not be in there but those were ones that jumped out at me.
Not right off the top of head but will check. I can work with you to get the changes intended in #98 into a patch.
But for now anyone who is going to do a next patch, start from the patch on #95 not #98.
Comment #103
miro_dietikerHow is this be in relation with the node edit sidebar?
Will we finally integrate this with manage form display, so items could be moved into the off canvas tray by common field configuration?
Comment #104
Gábor Hojtsy@miro_dietiker: this is only supposed to show up on frontend pages, so unless you do configure your node edit page to be on the front end (which core does allow you to do), they will not appear at the same time. If you do have node edit on the frontend, then they need to coexist. That said, it sounds unlikely that someone would go into editing some site elements when they are already editing a node, but it is not impossible.
Comment #105
tkoleary CreditAttribution: tkoleary at Acquia commentedRolled back to 95 and re-rolled css changes over that.
Comment #106
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #107
tkoleary CreditAttribution: tkoleary at Acquia commentedInterdiff was wrong but the patch is correct *I think*, checking.
Comment #108
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #109
SKAUGHTshortcut bar: edit on -- shortcuts disappear, but height is not reduced.
Currently, sometimes the entire bar is disappearing.
@dashes:
sorry, i'll try to rephrase my experience/concern.
when you click Edit -- all dashed appear.
but as you hover around, some disappear (lose :focus, maybe)
then, when click edit off. again dashes all appear.
however; and overall, you can always hover around and access 'quick edit' in context links.
turning off edit mode: some dashed still linger.
also: blocks with no Quick Edit context link still have dashed line -- these should not have this indicator.
EDIT:
context links have both 'quick edit' and 'configure block' -- i think this is confusing for general editors. why do we need both now?
IMO: I think we're acting too early in linking this tray to toolbar, when we have the base functionality of 'quickly editing block configurations in a tray'. When really, re-working toolbar should be approached a bit differently. This is feeling hacky to merge this way. As it stands now, we have a silent dependency on Editor (which gives the Edit button), Contextual and Toolbar.
Comment #112
SKAUGHTon 109 (myself): i see dependencies have been added to .info
@miro_dietiker
the node edit side bar is only a column on node forms (holding the administrative level fields of a node). whereas this sidebar is using a cleaned down form from block configurations. they are very different in that way.
In theory, yes that column could be moved into a tray..it is available to every theme. but at this time this tray isn't a core element.
Personally, I would say that then we are doing more to hide those important node edit fields, then helping those editing nodes by doing that. Whereas the tray does help someone in the front end complete editing without jumping into an admin theme. They are already editing full nodes in an admin theme (as most project use for node editing/creation).
Comment #113
miro_dietiker@Gábor Hojtsy and @SKAUGHT From developing Paragraphs and thinking about future editing UX, we need something like this. But we need it when editing the form.
The idea was to advanced settings about display of elements, but not have them as part of the main form.
So our idea is to annotate some form elements to show up in an off canvas area that can be opened and closed.
But i'm also thinking about comparing diffs and showing revision metadata and offering navigation buttons in that tray.
In the best case, the form display mode could allow a user to move fields into this area.
What do you think, can we make this tray support these use cases or we need some different tool?
Comment #114
tedbow@SKAUGHT re #109
Yes, this seems like problem right now we are just attaching the "focus" class which does not work because this will added/removed other wise.
I think this also is because of our reliance on the "focus" class.
All blocks will have the Quick Edit links. They all do not have them now because we haven't figured out how to handle the client side caching that the context module use to store contextual links. Right now the context module only refreshes this when the user's permission changes. You should see that all new blocks that you place get the new Quick Edit link. It is tricky to fix without a patch to the context module. Right now I think we should try to keep this patch to only the new experimental module.
Editor is not a depency. The Edit button is actually from the contextual module. I checked the code and uninstalled the Editor module just to be sure ;)
Toolbar and Contextual modules are hard dependencies since they are in the info.yml.
The linking of the toolbar functionality has been laid out in the parent issue: #2762505: Introduce "outside in" quick edit pattern to core and related issues such as #2732443: Finalize the behavior for triggering view/edit/build modes from the toolbar (and fix the "disappearing toolbar"). I think discussion about what should be include in the parent issue. Otherwise it will make this issue must more difficult to review and get done.
Yeah we probably don't need both. The mock ups in the parent issue I believe don't the the regular config. But they also add and "advanced settings" link in the Minimal Block form that is opened in the tray. Now that we have the Minimal Block form opening in the tray we should add the advanced link and remove the regular contextual link like you say. I can patch the BlockEntityOffCanvasForm because i also need to fix problem with it not saving the configuration.
Comment #115
SKAUGHTmiro_dietiker
generally, the tray is very open to being used in many contexts.. i believe tim.plunkett has added some aspects specifically for the basic ability to have form types. i think there's some limit at this point to work with blocks.. i'm not sure where the limit falls right now.. possibility #2763157: Allow plugins to provide multiple forms will be the rest of that answer.
Comment #116
tedbow@miro_dietiker re:
The tray should support other use cases than the ones explicitly added by the Outside in module. Right now we are just adding a new dialog type like we already have for modal.
Other modules could use this functionality but of course for now they would be dependent on an Experimental module. So I think long term if(when?) this module gets in as experimental we should think about taking the "offcanvas" dialog functionality out of Outside In into core like "modal" is today. But for now it makes more sense to keep all the functionality into 1 module so it can get in as experimental and have no or minimal effect on core. That way if after people start to try it out we find we want to change something major about the offcanvas tray we don't have worry at all about backwards compatibility. Experimentation FTW.
You can look at \Drupal\offcanvas_test\Controller\TestController to see how to use it.
Comment #117
SKAUGHT#114 tedbow:
thanks for overlooking. I know there's lots of changes and settling happening right now.
Comment #118
tedbowThis patch addresses a bunch of issues
Comment #119
tedbowCrossing off fixed items. These solutions could be improved later on but are working.
Comment #121
tedbowComment #123
tedbowThis patch
Comment #125
tim.plunkettRerolling this on top of #2763157: Allow plugins to provide multiple forms
Comment #127
tkoleary CreditAttribution: tkoleary at Acquia commentedPatch with CSS for the edit mode toolbar, interdiff coming
Comment #128
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #129
tim.plunkettFixing the merge and removing the non-existent CSS file
Comment #131
tedbowOk this
Removes machine name and region from minimal form.
Fixes a problem where the tray won't be removed correctly so would have trays.
And last but not least. Makes sure that our 1 JS test actually passes(fingers crossed)!!!! (we should extend that test to cover #2 above.
Comment #132
tim.plunkettReroll on the recent changes in #2763157: Allow plugins to provide multiple forms
Comment #133
tkoleary CreditAttribution: tkoleary at Acquia commentedPatch contains a lot of work on responsive CSS and cross-browser testing and debugging of transitions as well as some adjustments to transitions which I moved into a new file to make it easier to switch off for performance reasons.
It looks like a lot of stuff was removed from module,css, but actually most is still there but moved to media queries at the bottom.
Comment #134
tedbow@tkoleary changes for this file should not be in this patch. There are other files from Views, system, menu_ui and others that should not be here.
I am assigning this issue to myself to try to pull the changes out of #133. There is currently not a good patch to work with off this issue. I will try to upload fixed patch that has #132 and only intended changes from #133 with an hour or so.
Comment #135
tedbowOk this patch contains changes in #133 interdiff.txt which I think is correct.
It also contains a change from me.
Now not setting the block system_main_block as a Outside In editable because the Block contextual links are used on this block. So it was not working anyways.
Comment #136
tedbowOk this patch brings in changes from #2763157: Allow plugins to provide multiple forms
https://www.drupal.org/node/2763157#comment-11437931
With these changes I was able to make the new \Drupal\outside_in\Form\SystemBrandingOffCanvasForm
This extends the form for the Branding block to allow actually change the site name and slogan when you open the branding block in tray!
Comment #137
tim.plunkettApplying the interdiff from #2763157-24: Allow plugins to provide multiple forms and adjusting for those changes. Additionally cleaning up the code from the last couple patches.
Working on a new form for menu blocks now.
Comment #139
tim.plunkettThis adds an offcanvas form for menu blocks.
This has to workaround #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms), which I added @todos for.
Comment #140
tedbowOk, this patch updates offcanvas.js to work with the new menu block form. We just need to
Drupal.attachBehaviors($('#offcanvas'),drupalSettings);
After the tray is created and filled.
This also fixes a problem introduced in recently that caused the Branding block not save it's toggle values.
It adds the "Advance Options" link to the block forms in the tray to open the full block form.
Comment #143
tedbowIt appears the test failures in #140 were unrelated.
ok this patch adds JS test coverage for:
IMO this is enough test coverage for an experimental MVP. So I am not going to work on the tests anymore unless they break because of other changes or someone thinks we need more tests.
It also fixes changes how Drupal.attachBehaviors is called to:
Drupal.attachBehaviors(document.querySelector('#offcanvas'),drupalSettings);
@nod_ point out this is the correct way to do it.
Fixes call to plugin form in \Drupal\outside_in\Form\SystemBrandingOffCanvasForm::buildConfigurationForm
@tim.plunkett provided fix.
Comment #144
Bojhan CreditAttribution: Bojhan as a volunteer commentedFrom my point of view, the concept works. Which is what MVP is meant to do. So it can go in as Experimental.
This issue has been difficult to review, as it keeps changing completely over the course of days. Which is also exciting. Having reviewed the patch, there is definitely still a lot of wonky things (horizontal bars, white background, bartik styling everywhere, loads of help text). It is obviously worrisome, that most of these issues arise less than week before feature-freeze - but not sure what to do about that :) Planning wise something to look into, because most of this activity could have happend much longer ago.
It is worthwhile to look into how we can fast-track some of the visual optimizations before RC as outside-in has a certain sexy effects that right now aren't there :)
Looking forward to the follow-up list.
Comment #145
webchickSorry, I have to go take care of a sick kiddo, but here are the follow-ups we talked about. I'll get them incorporated into issue summaries a bit later.
* Figure out ideal editing scenario for blocks; e.g. whether/how to do visibility settings (must-have follow-up)
* Establish standard for quick editing in core (must-have follow-up)
* Adjust the colours of the sidebar so it’s not white on white on white. (should-have follow-up)
* Fixing the multiple scrolling (should-have follow-up)
* Fix various styling issues (required follow-up)
Comment #146
tkoleary CreditAttribution: tkoleary at Acquia commentedSome very hacky CSS to get the menu table to look ok. Will definitely need follow up work.
Comment #147
tim.plunkettUpdated with latest changes from #2763157: Allow plugins to provide multiple forms, which is very close.
Comment #148
tim.plunkettRerolled now that #2763157: Allow plugins to provide multiple forms went in!
Comment #150
tim.plunkettBad merge, sorry.
Comment #152
tedbowThis was getting the warning
Changed to expect a BlockPluginInterface and then added
We can't guarantee that the block will implement PluginWithFormsInterface but if not just returning the Block itself is fine because it will always implement PluginFormInterface. Also had to change \Drupal\block\BlockForm::getPluginForm because it was \Drupal\Core\Plugin\PluginFormFactoryInterface::createInstance which expects PluginWithFormsInterface. So changes to similar
This should use 'configure' not 'configuration'. Was causing test to fail.
Comment #153
tedbowOk this patch get rid of the PageInfo service. @tim.plunkett convinced me this is not very servicey.
Comment #154
xjmWe discussed this issue in light of the upcoming beta and agreed that it is a high-priority feature for 8.2.0, and so we agreed to consider it as a beta target, which can be committed after the first beta but ideally before the second.
However, this patch also includes changes to stable core code. Those changes must be made before the beta next week (so extracted from this patch and added in a beta dealine issue if they are still required and make sense on their own, and they need to or this is not experimental). :)
This issue still needs framework and release manager review so it is not a guarantee this issue will be committed during beta, just the decision to commit it once it is ready with those and other reviews and fixes.
Thanks everyone!
Comment #155
tedbowAdding #2774997: Make BlockForm extensible
This covers the changes to the block module.
It is only a change to 1 function.
I left out the phpdoc updates that were in this issue because were address current miss documented comments not changes related to this. So it affects 1 file instead of 3.
Comment #156
tedbowAdde #2775057: Make MenuForm extensible for the MenuForm logic
Comment #157
tedbow@xjm thanks for the comment above and the guidance on this.
The 2 above issue were committed (Thanks @alexpott!)
Here is a patch that only includes the outside_in module and the 1 line change to core/composer.json!
The interdiff is between #153 with the 8.2.x rebased in. So it only shows the core differences that were not included in #155 and #156. These were just phpdoc changes and variable name changes that are really a separate issue and not need for this module.
The outside_in module itself has not changed since #153
Update: Sorry made the interdiff a *.patch file :(
Comment #159
tim.plunkettComment #160
tim.plunkettSince #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms) went in first, we can remove the $form_state hacks I added.
Also went back through the entire patch to clean up any nitpicks.
Comment #161
tim.plunkettComment #162
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI reviewed this patch, and nothing jumps out at me as problematic to commit within an experimental module, so removing the "Needs framework manager review" tag. I'll post another comment with some nits later today or tomorrow.
Comment #163
effulgentsia CreditAttribution: effulgentsia at Acquia commentedx-posted with #161.
Comment #164
Wim LeersI read the entire issue, parent issue and patch.
@todo create contextual issue.
-> not yet done.core/misc/displace.js
. Inspect it with your browser's developer tools and you'll see this is not true. SKAUGHT is right that this introduced lots of edge cases and tricky things. It means that the "effectively available viewport" is smaller than the browser's viewport. Which means all JS/CSS needs to use/integrate withcore/misc/displace.js
. It also means you need to ensure that that is updated correctly, which in turn means you can have race conditions. It even means the browser's native "jump to anchor" navigation doesn't work automatically anymore.Why "offcanvas" and
#offcanvas
, but "main canvas" and#main-canvas
?Needs documentation to explain why this particular number was chosen.
camelCase class names is not something we do in Drupal's CSS.
These values seem rather specific.
Root-relative URL: this only works for Drupal sites that are not installed in a subdirectory. Use
../../../misc/icons/bebebe/ex.svg
Why do we need to repeat this? It's a child of
#offcanvas
, so it already has the same z-index?The toolbar module uses
em
andrem
, shouldn't this also?s/it's/its/
Also, we're not wrapping "the rest of the site".
That's a lot of different variations.
Can't we reduce this? Can we maybe use
calc()
to our advantage?The rest of core has omitted these prefixed variants years ago because those browsers actually unprefixed it a long time ago. This should do the same.
Then let's move these in a separate CSS file, so that it's easier to move it into Bartik once this module becomes non-experimental.
"performance" is too broad, this is specifically referring to browser rendering performance.
s/edit/contextual links toggle/
These selectors, and all others, are wrong. They should not target
.toolbar-icon-edit
, but.contextual-toolbar-tab
.Here, we suddenly do write "off canvas" and not "offcanvas"
Missing
\n
:@param
@param
and@return
Everywhere here.
AFAIK we don't have any other code in Drupal core that creates DOM elements like this.
Two spaces after
@param
every time, instead of one.s/html/HTML/
If it's a jQuery object, then use
{jQuery}
.All of this would greatly benefit from this being more structured, because state management for this looks like it will be painful. To provide the additional structure, without reinventing the wheel, the Contextual Links JS, Quick Edit JS, CKEditor toolbar configuration UI JS and Toolbar JS have all been written using Backbone models & views.
If that is not done, this absolutely needs sign-off from a JavaScript maintainer before this can be committed.
We haven't used event listeners like this one since #1342198: Use .on and .off instead of .bind, .unbind and .delegate.
This surely is not passing
eslint
. I get 3 errors, 1 warning. This is just one of those 4 things.Why include
div
in the selector?This code signals very strongly to me that we indeed want Backbone-based code. Because this looks a lot like the unmaintainable JS code of the past.
And this is the sort of "brittle DOM-based state tracking" that you can avoid by using Backbone.
s/Outside In/Outside-In/
s/access/change/
s/useful/common/ or s/useful/the most common/
This is overwriting
['#attached']
, rather than appending to['#attached']['library']
!Hah :)
Not convinced this is the appropriate long-term solution, but absolutely fine for experimental.
s/will remove/removes/
s/form/sforms/
s/id/ID/
s/$advance_url/$advanced_url/
Just use
$this->entity->toUrl('edit-form', ['query' => $query])
s/Options/options/
Now "OffCanvas", again something different.
The block plugin.
c/p remnant
Comment #165
GrandmaGlassesRopeMan@Wim Leers
I believe I've addressed some of the issues from your review in #164,
.click()
is just a shortcut for.on( "click", handler )
in most cases and.trigger( "click" )
when you want to trigger a click event.Comment #166
SKAUGHT#164
I think 'Quick Configure' is more apt a title than 'quick edit'. I think we're confusing in-place editing (content maintainer work) too closely to this kind of site-builder ability.
As this codebase/UX has been for stable (in it's self, to say) for a good week now..
Usability/UX direction. IMO the enhancements with toolbar, and the fact that the Contextual Links just give access to the tray seems like it's too much going on on the screen. The tray itself should be the change the user needs to be focused on.
Perhaps stepping back from Toolbar alterations could be a course todo?
changing status just to triggering tests
Comment #167
Bojhan CreditAttribution: Bojhan as a volunteer commentedThats possible, also should be something we fiddle on after commit. To me "Quick configure" might bias it away from editing things that people consider content, but are in the gray area between configuration and content.
None of the issues from https://www.drupal.org/node/2753941#comment-11443267 seem to be created yet, and place don't make placeholder issues - they are not helpful.
It looks like #12 definitely needs to be addressed. The UX should not be considered stable, there is a lot of work to be done to be close to the level thats desired - its just about being good enough to validate the potential with users.
Comment #168
SKAUGHTComment #169
tedbowadding related issue #2773591: New contextual links are not available after a module is installed
I previously created but forgot remove the todo.
Comment #170
Wim LeersFor extra context: proposed experimental modules are hard to review. The question is always "how much leniency do we need to grant?"
Is pretty PoC-quality code good enough? If so, commit this right now.
Or do we want it to have a clear plan towards stability, and signs of a complete architecture? If so, then we need more work.
Bojhan says in #167 — if so, then this is fine to commit. Because it's completely isolated.
Comment #171
SKAUGHTWim:
I think you're points about tabbing/esc, re-working with Displace amongst your other points help to point that more time to this as a base product is needed. Also, better Breakpoint integration. The Dark theme concept has gotten lost (perhaps other conversations have happened about that..)
This is the kind of tool that Drupal needs, but I do think 8.3 is more realistic for integration. POC and Modular Isolation isn't enough. Common Users need to be wow'd by this. It's not there yet.
I might think #2773591: New contextual links are not available after a module is installed to be a blocker, unless an install hook is added (if possible at all). Certainly with this, this module seems very broken to a Site Administrator activating this (even though it's listed as 'experimental') after a core update. any message with "please log out and clear your caches" is not a good user experience.
Comment #172
Wim LeersJust discussed this with Dries. I described my review's points and the difficulty of reviewing proposed experimental like I wrote in #170.
His high-level view on things shifted my perception of this patch.
Basically, his take is:
Now, looking at this patch from that perspective:
This is basically the API. It uses the ability for a plugin to have multiple forms, which was introduced by #2763157: Allow plugins to provide multiple forms (CR: https://www.drupal.org/node/2773829).
Therefore, the API surface is extremely limited. 100% of my review in #164 is actually criticizing internals, not the API. And yes, that includes usability, consistency, architecture, etc problems. But none of those problems require changes to the fundamental API.
Given that, moving back to NR. Not moving to RTBC, because this still
.Comment #173
tim.plunkettFixed more of Wim's nits. Some of the ones about CSS are still unaddressed, but are not blockers.
I agree with him (and Dries) about what it means for an experimental module to be "ready", and I think we're in a great place, and ready for commit pending review by a release manager.
Comment #174
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAll @todo's in this patch need an issue link.
Why scope this to
#main-canvas-wrapper
?Why require this to be a
div
?This todo applies only to part of what the function does. It should be moved to just that part.
s/outsideinedit/outsideInEdit/
Much of the contents of these CSS files seems to be more appropriate to be part of the
drupal.offcanvas
library rather than thedrupal.outside_in
library. Can those rules be split out accordingly and moved to the correct library?It's not casually obvious where 'outside-inblock-configure' comes from and why it doesn't have a hyphen between 'in' and 'block'. Can this be changed to something like:
That's abusing these hooks, and is a timebomb for malformed HTML if other modules adopt a similar pattern and the order in which tags are closed doesn't match the order in which they're opened. It also forces
div
where a theme might want to use a different tag for it. Could we instead set a #theme_wrappers for the 'page' type within hook_element_info_alter()?This is failing to apply cache contexts for the logic in _outside_in_apply_on_current_page(). For example, if you use the site default theme for your admin theme, then the block is render cached either with or without this class based on whether the first request is to an admin page or not.
Please expand, because it's not clear what should then be done based on that check.
I don't see why we need to return a
selector
property if the JS code doesn't make use of it.We shouldn't pass a fake selector in. We should either pass in a real one (like '#offcanvas') or NULL.
Seems like unnecessary duplication with the parent method. Can we instead call the parent method and then only change what needs changing (e.g., 'command')?
Would it be clearer to rename this to
$menu
?A lot of duplication with the parent method but HEAD's ModalRenderer has similar duplication. Perhaps a follow-up issue to clean up both?
Comment #175
tim.plunkettAFAICS none of those are blocking for an experimental module. I will try to fix as many of those as I can before commit, but this should still be reviewed by a release manager.
Comment #176
Gábor HojtsyAs per #172, this looks good as an experimental module other than the release manager review. The release manager review can/will happen as part of the RTBC review, so moving to RTBC.
For more details on what the agile process intends experimental modules to be, #2729061: [policy, no patch] Agile process for bigger changes in core (Part B: Implementation) has a nice figure. Here is a copy:
Basically they are anything from a minimal implementation of an idea through to perfected versions of that leading to finally meeting all the core gates and quality goals when it is promoted to a non-experimental module. So basically it should be added to core when we think we want to run with the idea even if it needs more work. Modules like this that are isolated enough to not cause any data issues are especially well suited for this process, so they are brought to users for validation and feedback as soon as possible. The idea is to fail fast and improve instead of working to death perfecting something in a hidden corner.
Comment #177
catchI tried to ignore anything that wasn't release-managery, as Wim says there's not a lot of API surface here. I wonder a bit why system menu block needs an entirely new form handler vs. say a form mode or similar - the unsetting of the specific form keys looks a bit odd there for example.
This is the bit that looks questionable.
Also while I tried to ignore non-release-managery things, I did notice this:
Looks like this is missing cache contexts - specifically it looks like it needs a new outside_in.apply cache context.
I also have a general concern with this module when combined with content_moderation - since they're more or less incompatible conceptually. That needs a follow-up opened.
Comment #178
xjmFollowing up on #176, these are the requirements an experimental module must fulfill:
https://www.drupal.org/core/experimental#requirements
Comment #179
webchickHm. I don't quite follow. Changes to content in-place editing is specifically out of scope of this patch/initiative. There shouldn't be anything that conflicts, beyond what already conflicts in HEAD.
Comment #180
tim.plunkettWRT menu block: it was decided that we needed to provide one concrete improvement to the existing block forms. Menu block was chosen, and the goal was to provide the means to edit the menu entity directly from the block.
Ideally the mechanism for presenting the menu tree in a form would be reusable in some way, but it is very much not. All of the logic is custom to MenuForm. So using it directly, and removing the ability to change other parts of the menu entity (label, ID, description) and removing the separate set of buttons was my solution.
If this approach is not good enough, I'd suggest we just remove it for this patch and keep the branding block improvement.
Also, to note: the branding block replacement does the same sort of thing:
As to the cache contexts, I'll take a look at that.
Comment #181
catchNot content moderation as such, but #2732081: WI: Phase G2: Full-site preview with Workspace UI. If you're in a workspace, you expect to be able to edit things without your live site being affected (and that's the entire point). So either you would edit cofig and it would immediately show up on the live site (seems confusing), or in place editing gets completely disabled (also confusing - why wouldn't you be able to preview a logo or site slogan change?).
Comment #182
xjm#2762505: Introduce "outside in" quick edit pattern to core has not been updated since July 8. Are all the followups that have been identified outlined there?
Comment #183
xjm@webchick, the interaction between this and the workflow initiative would be a part of the path to stable for both features. Not in scope here, but needs to be a checklist item in the plan for Outside In.
Comment #184
Bojhan CreditAttribution: Bojhan as a volunteer commentedFor this to go in we first need to properly identify and create the follow up issues outlined in https://www.drupal.org/node/2753941#comment-11443267.
Comment #186
tim.plunkettMoving back to 8.2.x since this is tagged 'beta target'.
Comment #187
tim.plunkettLearning cache contexts. Really unsure about what to do in here. Also, maybe _outside_in_apply_on_current_page() should be a service after all?
Comment #189
tim.plunkettAfter some discussion with @effulgentsia, moved _outside_in_apply_on_current_page() to a service.
Also, had to rename the cache context, the dots are meaningful to that system.
Comment #190
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think this can be changed to first check the 'tab' and only apply the cache context if there is.
#172 says that Dries cares about some level of API stability even in experimental modules. So seems like we should get the name of this interface and method nailed down. I think maybe s/OutsideInInfo/OutsideInManager/? And any thoughts on a better name than isApplied()? I like is_applied for the cache context, because that reflects a decision made elsewhere, but this interface is about making the decision itself.
Comment #191
webchick#182/#184: I believe the issue summary at #2762505: Introduce "outside in" quick edit pattern to core is now accurate. https://www.drupal.org/node/2762505/revisions/view/9952575/9954997
Comment #192
xjmThe current patch does not work when Drupal and Outside In are installed through the user interface (Standard/8.2.x). There are no contextual links to quick edit things.
The problem does not occur when I install with Drush. It persisted in a new incognito window, after uninstalling and reinstalling the module, after logging out and logging back in, after a full cache clear, and in an entirely different browser.
Comment #193
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYeah, the only reproducible way I found to get it to work is to do things in this order:
1: Enable the module
2: Do a full cache clear from the UI
3: Open an incognito window. The "Quick Edit" contextual links then appear within that incognito window.
The only way I found to get the contextual links to appear in the original window is to open up the Chrome console and run:
This issue already lists #2773591: New contextual links are not available after a module is installed as a related issue. Seems like maybe that needs to be a blocker for this one.
Yeah, I have not found this to make any difference for client-side contextual link caching despite what's said in the IS of #2773591: New contextual links are not available after a module is installed. But maybe there's something I'm missing?
When I opened up Safari (which I haven't tested a local D8 site with in many months), I didn't get any contextual links appearing at all (not even the regular "Configure block" ones). Not sure if this is a Safari bug or due to some very old stuff still client-side cached.
Comment #194
xjmNote that I do not believe #2773591: New contextual links are not available after a module is installed is actually a core bug, but rather a bug with this patch's functionality. I posted STR there.
Comment #195
GrandmaGlassesRopeManAfter some debugging it seems that the new Quick Edit contextual link does not always appear. You can check out, #2773591 for more information/context.
You can see that if the contextual link is missing, there is nothing available to execute a click action on.
However if the element is available in the contextual links menu then everything works smoothly.
Comment #196
tim.plunkettThis ensures the link appears after a cache clear. I never hit that because I was installing via Drush.
Comment #198
xjmSo, according to our UI text standards, this should be sentence-cased as "Quick edit". The title case "Quick Edit" would only refer to the name of something (like the stable module, for example).
UI text refinement is totally not blocking for an experimental module, but changing the E to lowercase seemed a bit silly to create a followup for, and it's easy to confirm the sentence case pattern for other operations in core.
Comment #199
tim.plunkettAgreed with #198, fixed.
Also the data-contextual-id changed due to the last change.
Comment #200
xjmI think this might actually be a separate issue than #2773591: New contextual links are not available after a module is installed. There is one issue where some contextual links apparently do not appear until after a cache clear, for links in new contextual groups, but another issue (the one fixed by this workaround) where links added to existing contextual groups don't show up even after a cache clear. Those two bugs may or may not have the same fix, but they would need two different tests, so I think we probably need a second issue and the comment needs to reference the new issue (since this workaround does not actually work around the need for the cache clear).
For me, this workaround is sufficient for the experimental module. The need for a manual cache clear is non-ideal because it may make existing site owners think the module is just broken when they try to test it, and if we can work around that too it'd be great, but that lesser problem is not a requirement before commit.
Edit: Not signing off yet because I'm still in the process of reviewing the roadmap. Also it looks like there is still some feedback earlier (I think?) that is not outlined in followup issues. Just saying this resolves my concern above. :)
Comment #201
xjmOh, for anyone who posted feedback earlier that was not incorporated in the patch here, please help out by filing a followup and posting it at #2762505: Introduce "outside in" quick edit pattern to core! Thanks.
Comment #202
tkoleary CreditAttribution: tkoleary at Acquia commentedCan we temporarily add an alert to clear caches in the DSM that appears on module install?
Comment #203
SKAUGHTbetter: clear caches on module enable.
Comment #204
xjmI filed a number of followup issues for things I do not believe need to block commit (some of these are minor but others are important):
@effulgentsia, @webchick, and I discussed making sure followups get filed for the other issues raised in comments but not addressed, especially those right before the RTBC. Any help with that is appreciated.
I do agree forcing a cache clear on enable would be a good workaround to add to the initial prototype. @effulgentsia was looking into how it could be fixed properly yesterday, but that was apparently quite the rabbit hole.
Comment #205
xjmHere are all the comments on this issue that may contain feedback not captured on the overall roadmap (which is #2762505: Introduce "outside in" quick edit pattern to core). Newest comments are first since some of them were definitely descoped from the experimental patch. On the older ones they may already be addressed but it was unclear to me.
Comment #206
xjmLOL.
Yeah, adding that to the required followups in #2762505: Introduce "outside in" quick edit pattern to core. ;)
Comment #207
xjmFollowing up on #199 through #203: Unfortunately, I did not test sufficiently yesterday with the latest patch. Installing the latest patch through the UI, the interaction behavior (dotted lines/gray hover) now works after installation, but only the site branding and search blocks open the sidebars. The other blocks do not, even after a manual cache clear.
@tim.plunkett is debugging this issue.
Comment #208
tim.plunkettcontextual.js caches contextual links in window.sessionStorage. If you open a new tab, everything will work. But there is no dedicated mechanism to clear this.
But, that cache is cleared if the permission hash is changed. We can force this by including a new permission, and assigning it to everyone who can administer blocks (the actual permission we care about right now).
outside_in_install() now flushes the persistent caches (just like ConfigTranslationDeleteForm) and assigns the permission.
Comment #209
dawehnerRight, this is exactly what #2773591: New contextual links are not available after a module is installed is saying.
Comment #210
nod_These should be named functions.
More importantly is there a reason to not use Drupal.theme? that's kinda exactly what it's for.
Doesn't seem like using jquery way of initializing element is greatly useful. We don't use that pattern anywhere else.
Would be better to have the html like everywhere else. The overhead is not needed here.
Also this doesn't integrate with our Drupal.displace() utility that is used to give the offsets of the different things on the page.
a data-offset-right attribute needs to be added.
The first part could be html directly but less important.
Needs to be initialized outside of the ajax command and documented. It's pretty much the only thing the module exposes in JS, it needs docs.
Any reason this is not in a behavior?
-parents +closest
named function please
Comment #211
tim.plunkettAll of those need to be made into follow-up issues, this is experimental code.
Comment #212
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWe don't need to flush all of them. We only need to invalidate the render and discovery caches. This patch updates to just that, and adds @todos linking to the relevant issues that it's working around.
We don't need this. The hook_block_view_alter() implementation added in #196 is a sufficient workaround for the client-side cache getting stale. This patch reverts the permission.
Comment #213
effulgentsia CreditAttribution: effulgentsia at Acquia commentedUpdating IS to clarify that the last issue listed in the "Known issues" section is now resolved. The new contextual link now shows up immediately after enabling the module.
Comment #214
Bojhan CreditAttribution: Bojhan as a volunteer commentedIf the known issues is resolved, remaining tasks and blockers - and most issues seem resolved we should be good to go.
Comment #215
DamienMcKenna(minor html fix to issue summary)
Comment #216
GrandmaGlassesRopeManAfter looking at #210 I've addressed some of the feedback,
Comment #217
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks. Back to "Needs review" for the interdiff in #216. Also, I'll post another patch shortly for some of the nits in #174.
Comment #218
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis only fixes points 3 and 5 from #174, since those are trivial. I'll open follow-ups for the rest.
Comment #219
xjmThanks @drpal for addressing @nod_'s feedback!
@webchick, @tim.plunkett, @tedbow, @effulgentsia, and I read back over the full issue looking for other unaddressed feedback and are in the process of filing followups or (for small cleanups) fixing them in the patch. Once we're done, this should cover everything I listed out in #205. I'm waiting on those being in place to give the plan a final overall review ensuring this module has a path to becoming stable, and keeping the "Needs release manager review" tag on until then.
Also updating issue credit for reviewers.
Comment #220
nod_@drpal: thanks for fixing #210, I'd have a few more things but I can live with a follow-up.
To add some context:
1. named function => debugging purposes.
2. the jQuery/$ function should be used only to transform a string or dom object into a jquery object. Nothing else. That will help make it possible to replace/swap out jquery in contrib.
Comment #221
webchickFinal target designs are in the issue summary of #2762505: Introduce "outside in" quick edit pattern to core, but to help streamline the initial patch we didn't try and tackle the styling issues here (in favour of #2781577: Properly style outside-in off canvas tray). Updated the issue summary with an accurate picture of how far this particular patch takes us so that doesn't surprise reviewers too much. :)
Comment #222
tedbowOk in this patch I went through all the remaining @todo's in the module and made sure they had a link to an issue. Most of these were new issues that I created , a couple were existing issues.
I also removed few todo's that I had originally put in that now has commented on and I now are not needed
. Since we only have 1 element we are ever replacing like modal we don't need the selector parameter.
. After talking with @tim.plunkett it seems it is ok to just test against two themes. This proves we are not targeting bartik markup. It also doesn't seems like the toolbar itself provides RTL for the admin menu which is what this would be similar too. Our markup doesn't change with RTL so the test would be the same.
I also a addressed a few small remaining issues from @Wim Leers' review in #164. These are from the second list under HR
9. added a @todo to #2784599: Outside In: Determine how to handle different breakpoints for the Off-canvas tray
12. fixed
13. fixed
20. fixed
28. fixed
Comment #223
tim.plunkettI've reviewed the last three interdiffs (and the patch itself to make sure it matches!) since the last time it was RTBC, and those all address the remaining in-scope feedback. Everything else has a follow-up created or described in the parent issue of #2762505: Introduce "outside in" quick edit pattern to core.
Therefore, setting this issue back to RTBC!
Comment #224
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust fixing some 80 character line wrapping and similar docs nits, so leaving at RTBC.
Comment #225
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAFAICT, that's correct: 38 issues currently tagged Outside-in. If someone's aware of any feedback that isn't reflected there, or in the IS of #2762505: Introduce "outside in" quick edit pattern to core, please flag that. Thanks.
Comment #226
xjmThank you to @tedbow, @tim.plunkett, @webchick, and @effulgentsia for going through all the comments on the issue with me and ensuring any unaddressed feedback was represented in a followup issue. I tagged a few more that did not have the tag, and added them to the roadmap in #2762505: Introduce "outside in" quick edit pattern to core.
Thanks also to @drpal for quickly addressing the JavaScript maintainer review!
Overall, this feature is really exciting. Now that the roadmap is finalized, it's clear that there is a lot of work ahead, but it is well-defined enough to give me confidence that it can be completed with the resources available. The module is independent of any other feature, with limited risk to stable features, security, or data integrity. The main point of potential conflict with stable functionality is Quick Edit, and reconciling its interactions with Outside In is part of the "must-have" followup work on the roadmap.
There are definitely some tricky usability problems to solve, but after testing the module and reviewing the roadmap, I think the next step is to have it available as an experimental module so that we can validate the idea and designs. My biggest concern is #2783509: Outside In's "Edit" toggle lets you do just about everything except edit your content, and it looks like that issue has been flagged as a priority for upcoming work.
In addition to the user experience work, the other major areas we need to address for this to be maintainable in the future are comprehensive documentation, full test coverage, and code quality, especially for the CSS and JS internals. Issues for those tasks are outlined in the must- and should-have sections of the roadmap. For the JS architecture, reviewers may wish to take note of #2784935: Use Backbone for client-side state management. For CSS, there is #2784437: Clean up CSS in outside_in module and a number of other issues. Finally, #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it is the next step for making the sidebar tray reusable as many people have suggested.
Based on all that and the followup work outlined in #2762505: Introduce "outside in" quick edit pattern to core, I'm comfortable signing off on this as an experimental module.
Comment #227
alexpottI've tested this functionality several times over the past weeks and I agree with @xjm this is an exciting change. Allowing users to configure blocks on the page they want to change is a fantastic way to show people the power of Drupal. Really nice work.
It is great to see that the feedback on the issue has either been addressed or has followups. We certainly have plenty to be getting on with and I'm sure we all look forward to the day this is enabled in the standard profile. It really is functionality users should get to use.
We got all the necessary signoff from the different product, release and framework managers - let's do this! Committed and pushed 42b81c6 to 8.3.x and abdf37c to 8.2.x. Thanks!
Some minor coding standard fixes on commit. The if logic changed order because we should do the cheapest thing first. Why instantiate a service when you don't have too?
Just an observation but this looks really fragile. Weights are always problematic. What happens if contrib or custom add something to the main canvas that has a weight higher or lower than the wrapper?
Comment #230
alexpottI've added the outside_in.module to the list of components - as well as block_place. Which actually is another piece of feedback - should outside_in and block_place be merged?
Comment #231
tedbow@alexpott thanks for committing!!!!!!! Also thanks to everyone who took part in this @tim.plunkett, @drpal, @tkoleary, @SKAUGHT, @effulgentsia, @xjm, @nod_, @webchick, @Bojhan, @catch and everyone who helped!
@alexpott re:
We do have this issue #2784495: Normalize block place and outside-in experiences
Initially talking with @tim.plunkett we were thinking about(though just first thoughts)
re:
This will change in #2784463: Convert outside_in_page_(top|bottom)() to a #theme_wrappers
Comment #232
webchickGasp! :D YAY!!!!! Thank you SO much!! And thanks too to everyone who helped so much getting this in!!! See you in the follow-ups. :D
Comment #233
xjmComment #234
xjmComment #235
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #236
naveenvalechaAdded change record for the Outside-In https://www.drupal.org/node/2786039
Comment #237
SKAUGHT#227
form #25. rather than using JS selectors and script to add wrapping that would never work across even 'average' contrib themes. Weights were set to 1000 to help reduce the possibility. Most modules would probably use natural sort, or a common number (ie 10), Of course, any module outweighing this is causing a break..they can alter the weight for whatever reason that are adding in..
Also, top and bottom are mainly JS additions.
Comment #239
Gábor HojtsyComment #240
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)