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)

gif

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.

  1. Move Quick Edit's "Edit" link to the far left of the Toolbar
  2. When clicked, triggers Edit mode
  3. Add "Quick Edit" to all contextual links that are editable; also triggers edit mode
  4. When clicked, sidebar opens, showing the target's configuration
  5. Except nodes, which trigger the existing "quick edit" functionality without opening sidebar
  6. Implement Edit button as View/Edit mode toggle
  7. Flip to secondary Toolbar with white background Styling could be improved - Edit icon disappears.
  8. Disable clickability of all menu / content links on page
  9. On mobile, outline all clickable regions on page - DELAYED: The existing edit mode seems to be broken for mobile in 8.2.x
  10. Trigger sidebar from clicking/tapping anywhere within a dotted region
  11. Open simplified Block form in side-bar. @see #2763157: Allow plugins to provide multiple forms
  12. Provide each simplified form in the tray with an “advanced” link that loads the full version of the form in the admin theme.

Known issues

  1. simplified Block form: doesn't save values
  2. "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

  1. #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
  2. #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',
    ];
CommentFileSizeAuthor
#235 outside-in-1-clip.gif1004.85 KBtkoleary
#224 interdiff.txt5.55 KBeffulgentsia
#224 2753941-outside_in-224.patch66.97 KBeffulgentsia
#222 interdiff-2753941-218-222.txt9.33 KBtedbow
#222 2753941-222.patch66.91 KBtedbow
#221 Screen Shot 2016-08-08 at 3.43.27 PM.png187.02 KBwebchick
#218 interdiff.txt1.14 KBeffulgentsia
#218 2753941-outside_in-218.patch66.51 KBeffulgentsia
#216 interdiff.txt5.93 KBGrandmaGlassesRopeMan
#216 2753941-outside_in-216.patch66.52 KBGrandmaGlassesRopeMan
#212 interdiff.txt1.5 KBeffulgentsia
#212 2753941-outside_in-212.patch66.19 KBeffulgentsia
#208 2753941-outside_in-209.patch66.49 KBtim.plunkett
#208 interdiff-2753941.txt1.28 KBtim.plunkett
#199 interdiff-2753941.txt1.85 KBtim.plunkett
#199 2753941-outside_in-199.patch65.21 KBtim.plunkett
#196 interdiff-2753941.txt9.34 KBtim.plunkett
#196 2753941-outside_in-196.patch65.19 KBtim.plunkett
#195 availablelinks.png210.45 KBGrandmaGlassesRopeMan
#195 missinglinks.png228.93 KBGrandmaGlassesRopeMan
#189 interdiff-2753941.txt8.97 KBtim.plunkett
#189 2753941-outside_in-189.patch64.63 KBtim.plunkett
#187 interdiff-2753941.txt2.79 KBtim.plunkett
#187 2753941-outside_in-187.patch60.14 KBtim.plunkett
#173 2753941-outside_in-173.patch58.77 KBtim.plunkett
#173 interdiff-2753941.txt16.02 KBtim.plunkett
#165 interdiff-2753941.txt4.14 KBGrandmaGlassesRopeMan
#165 2753941-outside_in-165.patch59.01 KBGrandmaGlassesRopeMan
#157 interdiff-2753941-153-157.patch3.09 KBtedbow
#157 2753941-157.patch59.54 KBtedbow
#153 interdiff-2753941-152-153.txt5.88 KBtedbow
#153 2753941-153.patch66.69 KBtedbow
#152 interdiff-2753941-150-152.txt2.28 KBtedbow
#152 2753941-152.patch68.21 KBtedbow
#150 interdiff.txt1014 bytestim.plunkett
#150 2753941-outside_in-150.patch67.17 KBtim.plunkett
#148 2753941-outside_in-148.patch67.63 KBtim.plunkett
#147 interdiff-new-changes.txt9.07 KBtim.plunkett
#147 interdiff-full.txt28.68 KBtim.plunkett
#147 2753941-outside_in-147.patch89.23 KBtim.plunkett
#146 interdiff-2753941-143-146.txt1.07 KBtkoleary
#146 2753941-outside_in-146.patch92.04 KBtkoleary
#143 interdiff-2753941-140-143.txt10.14 KBtedbow
#143 2753941-143.patch91.41 KBtedbow
#140 interdiff-2753941-139-140.patch2.81 KBtedbow
#140 2753941-140.patch86.06 KBtedbow
#139 interdiff.txt11.28 KBtim.plunkett
#139 2753941-outside_in-139.patch85.46 KBtim.plunkett
#137 2753941-outside_in-137.patch78.8 KBtim.plunkett
#137 interdiff-full.patch21.98 KBtim.plunkett
#137 interdiff-new-changes.txt9.68 KBtim.plunkett
#136 interdiff-2753941-135-136.txt8.53 KBtedbow
#136 2753941-136.patch75.04 KBtedbow
#135 interdiff-2753941-132-135.txt13.44 KBtedbow
#135 2753941-135.patch69.76 KBtedbow
#133 interdiff-2753941-132-133.txt12.73 KBtkoleary
#133 2753941-outside_in-133.patch77.92 KBtkoleary
#132 interdiff.txt15.72 KBtim.plunkett
#132 2753941-outside_in-132.patch66.04 KBtim.plunkett
#131 2753941-131.patch64.45 KBtedbow
#131 interdiff-2753941-129-131.txt4.29 KBtedbow
#129 interdiff.txt10.28 KBtim.plunkett
#129 2753941-outside_in-129.patch64.51 KBtim.plunkett
#128 interdiff-2753941-125-127.txt8.15 KBtkoleary
#127 2753941-outside_in-127.patch64.65 KBtkoleary
#125 2753941-outside_in-125.patch61.81 KBtim.plunkett
#125 interdiff-2753941.txt29.47 KBtim.plunkett
#123 interdiff-2753941-120-123.txt6.94 KBtedbow
#123 2753941-123.patch53.07 KBtedbow
#121 interdiff-2753941-118-120.txt2.22 KBtedbow
#121 2753941-120.patch50.55 KBtedbow
#118 interdiff-2753941-105-118.txt8.23 KBtedbow
#118 2753941-118.patch50.24 KBtedbow
#99 interdiff-2753941-outside_in-95-97.txt22.78 KBtkoleary
#98 2753941-outside_in-97.patch64.99 KBtkoleary
#95 toggle_toolbar.gif422.7 KBtedbow
#95 2753941-95.patch59.72 KBtedbow
#95 interdiff-2753941-93-95.txt2.94 KBtedbow
#93 interdiff-2753941.txt4.02 KBtim.plunkett
#93 2753941-outside_in-92.patch58.78 KBtim.plunkett
#90 2753941-outside_in-90.patch58 KBtim.plunkett
#90 interdiff-2753941.txt1.5 KBtim.plunkett
#88 visiblity,png.png248.79 KBSKAUGHT
#86 dashed-83.png229.82 KBSKAUGHT
#84 2753941-outside_in-84.patch58.36 KBtim.plunkett
#84 interdiff-2753941.txt31.21 KBtim.plunkett
#83 2753941-83.patch59.99 KBtedbow
#83 interdiff-2753941-82-83.txt2 KBtedbow
#82 2753941-82.patch58.62 KBGrandmaGlassesRopeMan
#82 interdiff-2753941-78-82.txt493 bytesGrandmaGlassesRopeMan
#78 2753941-78.patch62.69 KBSKAUGHT
#78 interdiff-2753941-77-78.txt1.76 KBSKAUGHT
#77 interdiff.txt2.26 KBGrandmaGlassesRopeMan
#77 2753941-77.patch53.61 KBGrandmaGlassesRopeMan
#69 interdiff-2753941-67-69.txt1.55 KBSKAUGHT
#69 2753941-69-reopen.patch55.46 KBSKAUGHT
#67 2753941-67.patch51.63 KBtedbow
#67 interdiff-2753941-66-67.txt2.52 KBtedbow
#66 interdiff-2753941-63-66.txt4.97 KBtkoleary
#66 2753941-66.patch50.18 KBtkoleary
#65 2753941-65.patch9.19 KBtkoleary
#63 interdiff-2753941-59-63.txt3.46 KBtedbow
#63 2753941-63.patch49.63 KBtedbow
#60 2753941-59.patch53.06 KBSKAUGHT
#60 interdiff-2753941-55-59.txt2 KBSKAUGHT
#55 2753941-55.patch52.88 KBSKAUGHT
#55 interdiff-2753941-51-55.txt2.72 KBSKAUGHT
#51 interdiff-2753941.txt20.3 KBtkoleary
#51 2753941-49.patch50.04 KBtkoleary
#49 interdiff-2753941-46-49.txt5.25 KBSKAUGHT
#49 2753941-49.patch41.93 KBSKAUGHT
#46 interdiff-2753941.txt2.88 KBtim.plunkett
#46 2753941-outside_in-46.patch38.27 KBtim.plunkett
#44 interdiff-2753941.txt13.48 KBtim.plunkett
#44 2753941-outside_in-43.patch38.24 KBtim.plunkett
#41 interdiff.txt3.19 KBtedbow
#41 2753941-41.patch26.33 KBtedbow
#37 Screenshot 2016-07-18 10.53.26.png9.71 KBtedbow
#37 interdiff.txt490 bytestedbow
#37 2753941-37.patch23.13 KBtedbow
#3 off-canvas-MVP.mov23.48 MBtkoleary
#3 off-canvas-MVP.gif2.43 MBtkoleary
#11 2753941-11.patch19.37 KBtedbow
#11 offcanvasopen.gif731.05 KBtedbow
#12 2753941-12.patch19.73 KBGrandmaGlassesRopeMan
#13 2753941-13.txt1.65 KBGrandmaGlassesRopeMan
#16 2753941-16.patch21.86 KBtedbow
#16 interdiff.txt3.56 KBtedbow
#19 2753941-19.patch22.07 KBtedbow
#19 interdiff.txt2.45 KBtedbow
#22 2753941-22.patch22.04 KBGrandmaGlassesRopeMan
#22 interdiff.txt1.73 KBGrandmaGlassesRopeMan
#24 2753941-jumps_to_btm.png69.74 KBSKAUGHT
#25 2753941-25.patch25.18 KBSKAUGHT
#25 interdiff-2753941-22-25.txt1.86 KBSKAUGHT
#30 2753941-30.patch23.12 KBandrewmacpherson
#30 interdiff-2753941-25-30.txt2.18 KBandrewmacpherson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tkoleary created an issue. See original summary.

Bojhan’s picture

Title: Create the "off canvas tray" for configuration » [Experimental] Create the "off canvas tray" for configuration
tkoleary’s picture

Issue summary: View changes
FileSize
23.48 MB
2.43 MB
tkoleary’s picture

Issue summary: View changes
miro_dietiker’s picture

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

tkoleary’s picture

This triggers a variable width for the main area, and requires very complex CSS variations (different break points varying .toolbar-tray-open).

I'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.

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.

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.

I usually argue agains modal, but an "Add" action dialog does not need to be displayed off main area. It can easily be modal.

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.

SKAUGHT’s picture

Just 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'

Bojhan’s picture

@Kevin/Ted Can you update the summary it does not reflect the scoping in the meta issue.

tkoleary’s picture

@skaught

Interesting points.

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.

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.

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'

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

tedbow’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
19.37 KB
731.05 KB

Ok 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:

'offcanvas_link_1' => [
        '#title' => 'Click Me 1!',
        '#type' => 'link',
        '#url' => Url::fromRoute('offcanvas_test.thing1'),
        '#attributes' => [
          'class' => ['use-ajax'],
          'data-dialog-type' => 'offcanvas',
        ],
        '#attached' => [
          'library' => [
            'outside_in/drupal.off_canvas',
          ],
        ],
      ],

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

  1. Currently the JS targets elements in the Bartik theme. Will need to be updated to be generic.
  2. Currently will not work with Contextual links because of #2764931: Contextual links don't work with 'use-ajax' links. There is a fix in that issue but I am having problem writing tests for it. There are currently JS test in core for contextual links.
GrandmaGlassesRopeMan’s picture

This should fix the animation issues with the patch from #11.

GrandmaGlassesRopeMan’s picture

FileSize
1.65 KB

The interdiff between #11 and #12.

tedbow’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
21.86 KB
3.56 KB

Ok this patch

  1. Adds hook_help with todo to actually add real description - was causing failing test
  2. Adds library to composer.json - was causing failing test
  3. Updates offcanvas.js to use response title instead of hard-coded title
  4. Fixes \Drupal\Tests\outside_in\Unit\Ajax\AjaxCommandsTest that was refering to old class name
  5. Adds Contextual links to quick edit blocks in offcanvas tray

Regarding the "Quick Edit" contextual links. Current problems:

  1. Don't work with ajax: #2764931: Contextual links don't work with 'use-ajax' links
  2. If you apply patch from #2764931 then you they open full form. For work on minimal block for see #2763157: Allow plugins to provide multiple forms
tedbow’s picture

tedbow’s picture

Fixes for composer.json, hook_help, OffCanvasDialogTest
Addressing test fails of #16

tedbow’s picture

Status: Needs work » Needs review
GrandmaGlassesRopeMan’s picture

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

SKAUGHT’s picture

Status: Needs work » Needs review

Trigger tests

SKAUGHT’s picture

Status: Needs review » Needs work
FileSize
69.74 KB

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

SKAUGHT’s picture

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

SKAUGHT’s picture

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

SKAUGHT’s picture

Status: Needs work » Needs review
SKAUGHT’s picture

Issue tags: +Accessibility
andrewmacpherson’s picture

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

  1. The close button for the tray needs to be a <button> element so it can be operated by keyboard.
  2. The close button needs an accessible label. I've used aria-labelfor now, but that isn't the only option. I guess the 'x' text will eventually be replaced with a nicer image icon?
  3. We need to provide a way for screen reader users to be able to find the tray. The <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 used role="region" and associated the <h1> as the accessible label.
  4. Since no page refresh occurs when opening and closing the tray, we need to inform a screen reader user about the change. I've added some Drupal.announce() calls. The exact message could be improved, depending on what accessible role and label we eventually give to the tray container element.

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 a role="region" for now.

azuracatprince’s picture

+  main_content_renderer.off_canvas:
+      class: Drupal\outside_in\Render\MainContent\OffCanvasRender
+      arguments: ['@title_resolver', '@renderer']
+      tags:
+        - { name: render.main_content_renderer, format: drupal_offcanvas }

This is a really nice idea. It gives an interesting opportunity to even render random modals as offcanvas always.


+#page.offCanvasDisplayInProgress {
+  position: fixed;
+  display: inline-block;
+}
+
+#page.offCanvasDisplayed {
+  display: inline-block;
+}

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.


+/**
+ * Performs tests on opening and manipulating dialogs via AJAX commands.
+ *
+ * @group Outside In
+ */
+class OffCanvasDialogTest extends AjaxTestBase {
+

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.

SKAUGHT’s picture

#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

tedbow’s picture

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"?

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

Are we only making this for right side. Shouldn't we be prepping ANY of the four sides?

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.

Also, rtf language: JS should swap sides with Lang switch

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?

tedbow’s picture

@azuracatprince

Just a random suggestion: I've heard we have actual javascript supported testing now, so what about actually testing this feature using it?

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.

tedbow’s picture

@SKAUGHT, I checked and switching to a RTL language does switch the manage menu position. So we should switch too.

SKAUGHT’s picture

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

tedbow’s picture

Here is patch that address problem noted in #31 about

+#page.offCanvasDisplayInProgress {
+  position: fixed;
+  display: inline-block;
+}
+
+#page.offCanvasDisplayed {
+  display: inline-block;
+}

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"

tedbow’s picture

Title: [Experimental] Create the "off canvas tray" for configuration » [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode
Issue summary: View changes
Parent issue: #2732443: Finalize the behavior for triggering view/edit/build modes from the toolbar (and fix the "disappearing toolbar") » #2762505: Introduce "outside in" quick edit pattern to core

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

tedbow’s picture

Issue summary: View changes

Added section on blocking issues and what they block.

tedbow’s picture

Status: Needs work » Needs review
FileSize
26.33 KB
3.19 KB

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
38.24 KB
13.48 KB

Added a couple interfaces, a service, an alter hook, and some tests.
Usage:

$form_object = \Drupal::service('outside_in.block.manager')->getFormObject($block, 'sidebar');
$form = $form_object->buildConfigurationForm($form, $form_state);
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
38.27 KB
2.88 KB

Removing usage of "sidebar"

Bojhan’s picture

Assuming we are not doing menu anymore? As @webchick noted in the other issue?

SKAUGHT’s picture

fixes 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

tkoleary’s picture

FileSize
50.04 KB
20.3 KB

Merged Tims changes with some new CSS.

Hadn't seen Skaughts changes in previous comment. Looks like a merge is needed.

tkoleary’s picture

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

SKAUGHT’s picture

i'll try to merge.

SKAUGHT’s picture

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

SKAUGHT’s picture

FileSize
2.72 KB
52.88 KB

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

SKAUGHT’s picture

Status: Needs work » Needs review

is CI running today? (:

tim.plunkett’s picture

+++ b/core/modules/outside_in/outside_in.module
@@ -46,11 +46,10 @@ function outside_in_contextual_links_view_alter(&$element, $items) {
-  $page_top['outside_in'] = [
+  $page_top['tray-open'] = [

@@ -59,11 +58,10 @@ function outside_in_page_top(array &$page_top) {
-  $page_top['outside_in'] = [
...
+  $page_bottom['tray-close'] = [

IMO this should be prefixed with the module name, and should use underscores and not hyphens.

outside_in_tray_[open|close] would be fine

SKAUGHT’s picture

Status: Needs work » Needs review
FileSize
2 KB
53.06 KB

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

SKAUGHT’s picture

ps: rtl needs more work. myself, i can put some time to that later today/this eve.

tedbow’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
49.63 KB
3.46 KB

Ok, 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.

tkoleary’s picture

FileSize
9.19 KB

This one resolves some issues with the close button as well as RTL and animation tweaks.

tkoleary’s picture

Rerolled patch

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.52 KB
51.63 KB

Ok this patch

  1. adds a new library "drupal.outside_in:" which is loaded if the "Edit" link in the toolbar is available
  2. Add a outside-in-editable class to all blocks. If the user has "administer blocks" permission and we aren't on an admin route(with @todo that we should actually check if there is different admin theme).
  3. Adds the contextual module as a dependency because this what make the "Edit" link in the toolbar.

I think changes in #66 break the current JS test but I am not sure why yet.

SKAUGHT’s picture

fixes secondary reopening of tray. one() event need to be on pageWrapper not offCanvasWrapper

SKAUGHT’s picture

Status: Needs work » Needs review
tedbow’s picture

Found 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

SKAUGHT’s picture

Currently, 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.

tkoleary’s picture

@skaught

I think the plan is to use breakpoint module as a dependency.

SKAUGHT’s picture

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

tkoleary’s picture

@skaught

i haven't played with breakpoint. i actually assume backbone was handling more there.. i'll have to do some more studying of it.

That would be awesome. I am equally unfamiliar with it. Jesse Beach wrote all that code when I first designed toolbar.

GrandmaGlassesRopeMan’s picture

FileSize
53.61 KB
2.26 KB

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

SKAUGHT’s picture

FileSize
1.76 KB
62.69 KB

retouch annouce() messages
remove return false on prototype

SKAUGHT’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Issue tags: +sprint

Marking for Usability sprint given activity :)

GrandmaGlassesRopeMan’s picture

This adds the ability to toggle the focus class on outside-in-editable items when using the edit button.

tedbow’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
31.21 KB
58.36 KB

Fixing a bunch of issues throughout, thanks to PHPStorm.
Also adding a route for the offcanvas block form.

SKAUGHT’s picture

FileSize
229.82 KB

#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.
SKAUGHT’s picture

#84
there is a deletion of some access_test stuff i think may unconnected here

SKAUGHT’s picture

FileSize
248.79 KB

@tkoleary (maybe)
after opening with 'quick edit' context link (yeppie..working!!)

  • if the sidebar has very tall content.. it's not scrollable downward right now.
  • the Visibility collapsible items are a bit of a mess. are theses suppose to be an like an accordion of items now?
Mixologic’s picture

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
@@ -0,0 +1,96 @@
+/**
+ * Tests the off-canvas tray functionality.
+ */
+class OffCanvasTest extends JavascriptTestBase {

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
58 KB

Thanks! Seems to have been it.
Also restored the files I accidentally deleted.

SKAUGHT’s picture

linking.
wonder how this will effect once in sidebar. hopefully just well.

otherwise to generally note:

  • it seems strange to have region select here. especially without being able to weight.
  • machine name is useless to all users.
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
58.78 KB
4.02 KB
tedbow’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
59.72 KB
422.7 KB

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

SKAUGHT’s picture

do 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??

tkoleary’s picture

@skaught

do 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??

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.

tkoleary’s picture

Refactored CSS to rely on Bartik, specific CSS can be a follow-up

tkoleary’s picture

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

tedbow’s picture

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

If anyone has tips on merging in individual files they would be helpful. Stackoverflow has not.

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.

miro_dietiker’s picture

How 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?

Gábor Hojtsy’s picture

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

tkoleary’s picture

Rolled back to 95 and re-rolled css changes over that.

tkoleary’s picture

tkoleary’s picture

Interdiff was wrong but the patch is correct *I think*, checking.

tkoleary’s picture

FileSize
15.72 KB
SKAUGHT’s picture

Status: Needs work » Needs review

shortcut 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?

  • swap order (quick on top)
  • rename 'configure block' to 'Full Config'

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.

SKAUGHT’s picture

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

miro_dietiker’s picture

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

tedbow’s picture

@SKAUGHT re #109

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.

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.

turning off edit mode: some dashed still linger.

I think this also is because of our reliance on the "focus" class.

also: blocks with no Quick Edit context link still have dashed line -- these should not have this indicator.

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.

As it stands now, we have a silent dependency on Editor (which gives the Edit button), Contextual and Toolbar.

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.

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

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.

context links have both 'quick edit' and 'configure block' -- i think this is confusing for general editors. why do we need both now?

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.

SKAUGHT’s picture

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

tedbow’s picture

@miro_dietiker re:

What do you think, can we make this tray support these use cases or we need some different tool?

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.

'offcanvas_link_1' => [
        '#title' => 'Click Me 1!',
        '#type' => 'link',
        '#url' => Url::fromRoute('offcanvas_test.thing1'),
        '#attributes' => [
          'class' => ['use-ajax'],
          'data-dialog-type' => 'offcanvas',
        ],
        '#attached' => [
          'library' => [
            'outside_in/drupal.off_canvas',
          ],
        ],
      ],
SKAUGHT’s picture

#114 tedbow:
thanks for overlooking. I know there's lots of changes and settling happening right now.

tedbow’s picture

This patch addresses a bunch of issues

  1. created getItemsToToggle() js function
  2. Changed js-outsidein-edit-mode to js-outside-in-edit-mode.
  3. added hover color to editable items
  4. changed .content to .offcanvas-content
  5. Remove use of focus for highlighting.
  6. Changed #canvas-tray to #main-canvas - was confusing
  7. Change "view" to "editing" in toolbar when in edit mode
  8. remove use of focus for highlighting. this was causing problems b/c core also uses this class and toggles it. Now css targets #main-canvas.js-outside-in-edit-mode .outside-in-editable. I wrote some css to put dashed border but I am sure it should be changed
  9. moved css to outside_in library so available for first animation
tedbow’s picture

Issue summary: View changes

Crossing off fixed items. These solutions could be improved later on but are working.

tedbow’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
50.55 KB
2.22 KB
  1. Moving "Edit" to left in toolbar
  2. Toolbar becomes white when in edit mode. Someone with css skills should make it look better.
tedbow’s picture

Status: Needs work » Needs review
FileSize
53.07 KB
6.94 KB

This patch

  1. Closes the "Manage" toolbar is open when going into edit mode
  2. Triggers "Editing" mode when click "Quick Edit" if edit mode is not currently active
  3. Doesn't load the Outside In JS if the admin theme is being used. The Contextual Edit mode is not active on these pages
  4. Changes the toolbar to darkgrey when in Edit Mode. I am not a designer so there is probably another fix but the white look really bad with the white tray. In the mockups the tray was darker so I think either have to make the tray dark or adjust the toolbar in edit mode.
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
29.47 KB
61.81 KB
tkoleary’s picture

Patch with CSS for the edit mode toolbar, interdiff coming

tkoleary’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
64.51 KB
10.28 KB

Fixing the merge and removing the non-existent CSS file

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
64.45 KB

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

tim.plunkett’s picture

tkoleary’s picture

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

tedbow’s picture

Assigned: Unassigned » tedbow
Status: Needs review » Needs work
+++ b/core/composer.json
@@ -111,6 +111,7 @@
diff --git a/core/core.services.yml b/core/core.services.yml

+++ b/core/modules/outside_in/tests/src/Unit/Ajax/OpenOffCanvasDialogCommandTest.php
diff --git a/core/modules/system/migration_templates/menu.yml b/core/modules/system/migration_templates/menu.yml
index 974a2d6..542faa4 100644

index 974a2d6..542faa4 100644
--- a/core/modules/system/migration_templates/menu.yml

--- a/core/modules/system/migration_templates/menu.yml
+++ b/core/modules/system/migration_templates/menu.yml

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

tedbow’s picture

Assigned: tedbow » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
69.76 KB
13.44 KB

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

tedbow’s picture

Ok 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!

tim.plunkett’s picture

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

tim.plunkett’s picture

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

tedbow’s picture

Ok, 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
91.41 KB
10.14 KB

It appears the test failures in #140 were unrelated.

ok this patch adds JS test coverage for:

  1. Clicking "Edit" in the toolbar and entering edit mode
  2. Opening block form by clicking block not "Quick Edit" link when in Editing mode
  3. Advanced block form elements, visibility, region, aren't present.
  4. Opening branding block form and changing the site name on save.
  5. Opening the "Powered by Drupal" block form and changing the block label

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.

Bojhan’s picture

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

webchick’s picture

Sorry, 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)

tkoleary’s picture

Some very hacky CSS to get the menu table to look ok. Will definitely need follow up work.

tim.plunkett’s picture

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
67.17 KB
1014 bytes

Bad merge, sorry.

tedbow’s picture

Status: Needs work » Needs review
FileSize
68.21 KB
2.28 KB
  1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,74 @@
    +  protected function getPluginForm(PluginWithFormsInterface $block) {
    

    This was getting the warning

    Warning: Declaration of Drupal\outside_in\Block\BlockEntityOffCanvasForm::getPluginForm(Drupal\Core\Plugin\PluginWithFormsInterface $block) should be compatible with Drupal\block\BlockForm::getPluginForm(Drupal\Core\Block\BlockPluginInterface $block) in require() (line 13 of /var/www/d8_2_ux/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php).

    Changed to expect a BlockPluginInterface and then added

    if ($block instanceof PluginWithFormsInterface) {
          return $this->pluginFormFactory->createInstance($block, 'offcanvas', 'configure');
        }
        return $block;

    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

      protected function getPluginForm(BlockPluginInterface $block) {
        if ($block instanceof PluginWithFormsInterface) {
          return $this->pluginFormFactory->createInstance($block, 'configure');
        }
        return $block;
      }
  2. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,74 @@
    +    return $this->pluginFormFactory->createInstance($block, 'offcanvas', 'configuration');
    

    This should use 'configure' not 'configuration'. Was causing test to fail.

tedbow’s picture

Ok this patch get rid of the PageInfo service. @tim.plunkett convinced me this is not very servicey.

xjm’s picture

Category: Task » Feature request
Priority: Normal » Major
Issue tags: +beta target

We 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!

tedbow’s picture

Adding #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.

tedbow’s picture

Adde #2775057: Make MenuForm extensible for the MenuForm logic

tedbow’s picture

@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 :(

tim.plunkett’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Since #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.

tim.plunkett’s picture

Issue tags: +JavaScript
effulgentsia’s picture

Issue tags: -Needs framework manager review, -JavaScript

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

effulgentsia’s picture

Issue tags: +JavaScript

x-posted with #161.

Wim Leers’s picture

Status: Needs review » Needs work

I read the entire issue, parent issue and patch.

  1. From the IS: @todo create contextual issue. -> not yet done.
  2. The video embedded in the IS looks nothing like what you get when applying the patch? Should be made clear in the IS.
  3. #6: 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. — this is not true. If it were true, we wouldn't need 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 with core/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.
  4. Accessibility: This does not yet meet the accessibility gate! Tabbing should jump from one editable to the next one, but it just does the "normal" tabbing. This should use the TabbingManager to constrain tabbing to just the contextual links, just like Contextual Links module does. But also… and perhaps more worrisome… this is then overriding that portion of Contextual Links? Why?
  5. Accessibility: Another accessibility problem is that you can't stop quick editing by pressing ESC.
  6. Usability: Quick Edit should be listed before Configure block
  7. Usability: It should be Quick edit, not Quick Edit
  8. Usability: While quick editing a block, you can click Quick edit again
  9. Usability: While "edit mode" is on, you can click on a block to start quick editing it. But you can still "click through" to interact with the block itself, so if you're not careful, you'll suddenly find yourself navigating elsewhere
  10. It's confusing that this uses the action label "Quick edit", but it's provided by the "Outside-In" module, not by the "Quick Edit" module. Should this be called "Quick configure"?
  11. Code: lack of consistency.
  12. Code: this code looks very rough. Which makes it all the more important to have all the necessary follow-ups filed as per https://www.drupal.org/core/experimental, so committers/community have a fair sense of how much work remains.

  1. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +/* Position the offcanvas tray container outside the right of the viewport. */
    +#offcanvas {
    ...
    +/* Shift the main canvas to the right for right-to-left languages. */
    +[dir=“rtl”] #main-canvas-wrapper.js-tray-open #main-canvas {
    

    Why "offcanvas" and #offcanvas, but "main canvas" and #main-canvas?

  2. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +  z-index: 501;
    

    Needs documentation to explain why this particular number was chosen.

  3. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +#offcanvas > button.offcanvasClose {
    

    camelCase class names is not something we do in Drupal's CSS.

  4. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +  height: 52px;
    +  width: 40px;
    

    These values seem rather specific.

  5. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +  background: url(/core/misc/icons/bebebe/ex.svg) center center no-repeat;
    

    Root-relative URL: this only works for Drupal sites that are not installed in a subdirectory. Use ../../../misc/icons/bebebe/ex.svg

  6. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +  z-index: 501;
    

    Why do we need to repeat this? It's a child of #offcanvas, so it already has the same z-index?

  7. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +@media (max-width: 700px) {
    ...
    +    width: 300px;
    +    margin-right: -300px;
    +    padding-top: 39px;
    

    The toolbar module uses em and rem, shouldn't this also?

  8. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +  /* Wrap the rest of the site so we can control it’s width. */
    

    s/it's/its/

    Also, we're not wrapping "the rest of the site".

  9. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +@media (min-width: 700px) {
    ...
    +@media (min-width: 900px) {
    ...
    +@media (min-width: 1000px) {
    

    That's a lot of different variations.

    Can't we reduce this? Can we maybe use calc() to our advantage?

  10. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    +    -moz-transform: translateX(-100%);
    +    -o-transform: translateX(-100%);
    

    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.

  11. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    + * Form layout changes, mostly specific to Bartik theme and menu.
    

    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.

  12. +++ b/core/modules/outside_in/css/outside_in.motion.css
    @@ -0,0 +1,68 @@
    + * to improve performance if desired.
    

    "performance" is too broad, this is specifically referring to browser rendering performance.

  13. +++ b/core/modules/outside_in/css/outside_in.theme.css
    @@ -0,0 +1,104 @@
    +/* Style the edit tab in the toolbar. */
    

    s/edit/contextual links toggle/

  14. +++ b/core/modules/outside_in/css/outside_in.theme.css
    @@ -0,0 +1,104 @@
    +button.toolbar-icon.toolbar-icon-edit.toolbar-item {
    ...
    +button.toolbar-icon.toolbar-icon-edit.toolbar-item:hover,
    +button.toolbar-icon.toolbar-icon-edit.toolbar-item:focus {
    

    These selectors, and all others, are wrong. They should not target .toolbar-icon-edit, but .contextual-toolbar-tab.

  15. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    + * Drupal’s off canvas library.
    ...
    +   * Create the title element for the off canvas element.
    ...
    +   *   jQuery object that is the off canvas title element.
    

    Here, we suddenly do write "off canvas" and not "offcanvas"

  16. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +  /**
    +   * Create a wrapper container for the off canvas element.
    +   * @param  {number} pageWidth
    +   *   The width of #page-wrapper.
    +   * @return {object}
    +   *   jQuery object that is the off canvas wrapper element.
    +   */
    

    Missing \n:

    1. Between the short description and the first @param
    2. Between the last @param and @return

    Everywhere here.

  17. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +  var createOffCanvasWrapper = function (pageWidth) {
    +   return $('<div />', {
    +     'id': 'offcanvas',
    +     'role': 'region',
    +     'aria-labelledby': 'offcanvas-header'
    +   });
    ...
    +  var createTitle = function (title) {
    +    return $('<h1 />', {text: title, id: 'offcanvas-header'});
    +  };
    

    AFAIK we don't have any other code in Drupal core that creates DOM elements like this.

  18. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +   * @param  {string} title
    ...
    +   * @param  {object} offCanvasWrapper
    ...
    +   * @param  {object} pageWrapper
    

    Two spaces after @param every time, instead of one.

  19. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +   *   This is fully rendered html from Drupal.
    

    s/html/HTML/

  20. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +   * @return {object}
    +   *   jQuery object that is the off canvas content element.
    

    If it's a jQuery object, then use {jQuery}.

  21. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +  var createOffCanvasContent = function (data) {
    ...
    +  var createOffCanvasClose = function (offCanvasWrapper, pageWrapper) {
    ...
    +  Drupal.AjaxCommands.prototype.openOffCanvas = function (ajax, response, status) {
    +    // Discover display/viewport size.
    +    // @todo Work in breakpoints for tray size.
    +    var $pageWrapper = $('#main-canvas-wrapper');
    +    var pageWidth = $pageWrapper.width();
    +
    +    // Set the initial state of the off canvas element.
    +    // If the state has been set previously, use it.
    +    Drupal.offCanvas = {
    +      visible: (Drupal.offCanvas ? Drupal.offCanvas.visible : false)
    +    };
    +
    +    // Construct off canvas wrapper
    +    var $offcanvasWrapper = createOffCanvasWrapper(pageWidth);
    +
    +    // Construct off canvas internal elements.
    +    var $offcanvasClose = createOffCanvasClose($offcanvasWrapper, $pageWrapper);
    +    var $title = createTitle(response.dialogOptions.title);
    +    var $offcanvasContent = createOffCanvasContent(response.data);
    

    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.

  22. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +    }).click(function () {
    
    +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +    .click(function (e) {
    

    We haven't used event listeners like this one since #1342198: Use .on and .off instead of .bind, .unbind and .delegate.

  23. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,126 @@
    +    Drupal.attachBehaviors(document.querySelector('#offcanvas'),drupalSettings);
    

    This surely is not passing eslint. I get 3 errors, 1 warning. This is just one of those 4 things.

  24. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +  $('div.contextual-toolbar-tab.toolbar-tab button').click(function () {
    

    Why include div in the selector?

  25. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +    data.$el.find('.outside-inblock-configure a').click(function () {
    +      if (!isActiveMode()) {
    +        $('div.contextual-toolbar-tab.toolbar-tab button').click();
    +      }
    +    });
    

    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.

  26. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +  var isActiveMode = function () {
    +    return $('#toolbar-bar').hasClass('js-outside-in-edit-mode');
    +  };
    

    And this is the sort of "brittle DOM-based state tracking" that you can avoid by using Backbone.

  27. +++ b/core/modules/outside_in/outside_in.info.yml
    @@ -0,0 +1,10 @@
    +name: 'Outside In'
    

    s/Outside In/Outside-In/

  28. +++ b/core/modules/outside_in/outside_in.info.yml
    @@ -0,0 +1,10 @@
    +description: 'Provides the ability to access useful configuration from the Drupal front-end.'
    

    s/access/change/
    s/useful/common/ or s/useful/the most common/

  29. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,140 @@
    +    $element['#attached'] = [
    +      'library' => [
    +        'outside_in/drupal.off_canvas',
    +      ],
    +    ];
    

    This is overwriting ['#attached'], rather than appending to ['#attached']['library']!

  30. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,140 @@
    +function outside_in_page_top(array &$page_top) {
    +  // Opens a div for consistent wrapping to {{ page }} render in all themes.
    +  $page_top['outside_in_tray_open'] = [
    +    '#markup' => '<div id=“main-canvas-wrapper”><div id=“main-canvas”>',
    +    '#weight' => 1000,
    +  ];
    +}
    +
    +/**
    + * Implements hook_page_bottom().
    + */
    +function outside_in_page_bottom(array &$page_bottom) {
    +  // Closes a div for consistent wrapping to {{ page }} render in all themes.
    +  $page_bottom['outside_in_tray_close'] = [
    +    '#markup' => '</div></div>',
    +    '#weight' => -1000,
    +  ];
    +}
    

    Hah :)

    Not convinced this is the appropriate long-term solution, but absolutely fine for experimental.

  31. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,81 @@
    + * This form will remove advanced sections of regular block form such as the
    

    s/will remove/removes/
    s/form/sforms/

  32. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,81 @@
    + * visibility settings, machine id and region.
    

    s/id/ID/

  33. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,81 @@
    +    $advance_url = Url::fromRoute(
    

    s/$advance_url/$advanced_url/

  34. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,81 @@
    +      'entity.block.edit_form',
    +      [
    +        'block' => $this->entity->id(),
    +      ]
    +    );
    +
    +    if ($destination = $this->getRequest()->query->has('destination')) {
    +      $query['destination'] = $this->getRequest()->query->get('destination');
    +      $advance_url->setOption('query', $query);
    +    }
    

    Just use $this->entity->toUrl('edit-form', ['query' => $query])

  35. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
    @@ -0,0 +1,81 @@
    +      '#title' => $this->t('Advanced Options'),
    

    s/Options/options/

  36. +++ b/core/modules/outside_in/src/Form/SystemBrandingOffCanvasForm.php
    @@ -0,0 +1,102 @@
    + * The OffCanvas form handler for the SystemBrandingBlock.
    

    Now "OffCanvas", again something different.

  37. +++ b/core/modules/outside_in/src/Form/SystemBrandingOffCanvasForm.php
    @@ -0,0 +1,102 @@
    +   * The plugin.
    

    The block plugin.

  38. +++ b/core/modules/outside_in/src/Render/MainContent/OffCanvasRender.php
    @@ -0,0 +1,63 @@
    +    // Attach the library necessary for using the OpenModalDialogCommand and set
    

    c/p remnant

GrandmaGlassesRopeMan’s picture

@Wim Leers

I believe I've addressed some of the issues from your review in #164,

  • 15, 16 - Fixed.
  • 17 - I don't really see an issue with this and it creates a easily understandable block for creating elements.
  • 18, 19, 20 - Fixed.
  • 21 - Yes. I absolutely agree that this work should be encapsulated with Backbone models & views.
  • 22 - Fixed, although .click() is just a shortcut for .on( "click", handler ) in most cases and .trigger( "click" ) when you want to trigger a click event.
  • 23 - Yeah, I fixed this. Some editors possibly do not constant check against eslint rules. ¯\_(ツ)_/¯
  • 24 - Fixed.
  • 25, 26 - See my comment for 21.
SKAUGHT’s picture

Status: Needs work » Needs review

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

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

SKAUGHT’s picture

Status: Needs review » Needs work
tedbow’s picture

Wim Leers’s picture

For 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 its just about being good enough to validate the potential with users. — if so, then this is fine to commit. Because it's completely isolated.

SKAUGHT’s picture

Wim:
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.
Wim Leers’s picture

Status: Needs work » Needs review

Just 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:

API stability matters more than internals like CSS + JS. It's important that contrib module authors start experimenting with this, so we can make it non-experimental in the not too distant future. Therefore the only thing that truly matters here is the API being stable. Or us at least expecting that only minor changes will be necessary.

Now, looking at this patch from that perspective:

+++ b/core/modules/outside_in/outside_in.module
@@ -0,0 +1,140 @@
+/**
+ * Implements hook_block_alter().
+ */
+function outside_in_block_alter(&$definitions) {
+  if (!empty($definitions['system_branding_block'])) {
+    $definitions['system_branding_block']['forms']['offcanvas'] = SystemBrandingOffCanvasForm::class;
+  }
+
+  // Since menu blocks use derivatives, check the definition ID instead of
+  // relying on the plugin ID.
+  foreach ($definitions as &$definition) {
+    if ($definition['id'] === 'system_menu_block') {
+      $definition['forms']['offcanvas'] = SystemMenuOffCanvasForm::class;
+    }
+  }
+}

+++ b/core/modules/outside_in/src/Form/SystemBrandingOffCanvasForm.php
@@ -0,0 +1,102 @@
+class SystemBrandingOffCanvasForm extends PluginFormBase implements ContainerInjectionInterface {

+++ b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
@@ -0,0 +1,137 @@
+class SystemMenuOffCanvasForm extends PluginFormBase implements ContainerInjectionInterface {

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 Needs release manager review.

tim.plunkett’s picture

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

effulgentsia’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/outside_in/css/outside_in.module.css
    @@ -0,0 +1,200 @@
    + * @todo Move changes into toolbar module when outside-in is not experimental.
    

    All @todo's in this patch need an issue link.

  2. +++ b/core/modules/outside_in/css/outside_in.motion.css
    @@ -0,0 +1,68 @@
    +#main-canvas-wrapper .contextual,
    +#main-canvas-wrapper .outside-in-editable,
    

    Why scope this to #main-canvas-wrapper?

  3. +++ b/core/modules/outside_in/css/outside_in.theme.css
    @@ -0,0 +1,104 @@
    +div#offcanvas {
    +++ b/core/modules/outside_in/css/outside_in.theme.css
    @@ -0,0 +1,104 @@
    +[dir="rtl"] div#offcanvas {
    

    Why require this to be a div?

  4. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +   * @todo Fix contextual links to work with use-ajax links in
    +   *   https://www.drupal.org/node/2764931.
    

    This todo applies only to part of what the function does. It should be moved to just that part.

  5. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +  Drupal.behaviors.outsideinedit = {
    

    s/outsideinedit/outsideInEdit/

  6. +++ b/core/modules/outside_in/outside_in.libraries.yml
    @@ -0,0 +1,23 @@
    +      css/outside_in.module.css: {}
    +      css/outside_in.theme.css: {}
    +      css/outside_in.motion.css: {}
    

    Much of the contents of these CSS files seems to be more appropriate to be part of the drupal.offcanvas library rather than the drupal.outside_in library. Can those rules be split out accordingly and moved to the correct library?

  7. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,136 @@
    +  if (isset($element['#links']['outside-inblock-configure'])) {
    

    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:

    $class = Html::getClass('outside_in.block_configure');
    if (isset($element['#links'][$class])) {
    
  8. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,136 @@
    +/**
    + * Implements hook_page_top().
    + */
    +function outside_in_page_top(array &$page_top) {
    +  // Opens a div for consistent wrapping to {{ page }} render in all themes.
    +  $page_top['outside_in_tray_open'] = [
    +    '#markup' => '<div id="main-canvas-wrapper"><div id="main-canvas">',
    +    '#weight' => 1000,
    +  ];
    +}
    +
    +/**
    + * Implements hook_page_bottom().
    + */
    +function outside_in_page_bottom(array &$page_bottom) {
    +  // Closes a div for consistent wrapping to {{ page }} render in all themes.
    +  $page_bottom['outside_in_tray_close'] = [
    +    '#markup' => '</div></div>',
    +    '#weight' => -1000,
    +  ];
    +}
    

    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()?

  9. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,136 @@
    +  if (_outside_in_apply_on_current_page() && $variables['plugin_id'] !== 'system_main_block') {
    +    // Add class to all blocks to allow Javascript to target.
    +    $variables['attributes']['class'][] = 'outside-in-editable';
    +  }
    

    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.

  10. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,136 @@
    +  // @todo Check if there is actually a different admin theme.
    

    Please expand, because it's not clear what should then be done based on that check.

  11. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
    @@ -0,0 +1,54 @@
    +   * @todo Do we need a selector? Or act the same as modal?
    

    I don't see why we need to return a selector property if the JS code doesn't make use of it.

  12. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
    @@ -0,0 +1,54 @@
    +    parent::__construct('#drupal-offcanvas', $title, $content, $dialog_options, $settings);
    

    We shouldn't pass a fake selector in. We should either pass in a real one (like '#offcanvas') or NULL.

  13. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
    @@ -0,0 +1,54 @@
    +    return [
    +      'command' => 'openOffCanvas',
    +      'selector' => $this->selector,
    +      'settings' => $this->settings,
    +      'data' => $this->getRenderedContent(),
    +      'dialogOptions' => $this->dialogOptions,
    +    ];
    

    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')?

  14. +++ b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
    @@ -0,0 +1,137 @@
    +  /**
    +   * @var \Drupal\system\MenuInterface
    +   */
    +  protected $entity;
    

    Would it be clearer to rename this to $menu?

  15. +++ b/core/modules/outside_in/src/Render/MainContent/OffCanvasRender.php
    @@ -0,0 +1,63 @@
    +  public function renderResponse(array $main_content, Request $request, RouteMatchInterface $route_match) {
    +    $response = new AjaxResponse();
    +
    +    // First render the main content, because it might provide a title.
    +    $content = $this->renderer->renderRoot($main_content);
    +
    +    // Attach the library necessary for using the OpenOffCanvasDialogCommand and
    +    // set the attachments for this Ajax response.
    +    $main_content['#attached']['library'][] = 'outside_in/drupal.off_canvas';
    +    $response->setAttachments($main_content['#attached']);
    +
    +    // If the main content doesn't provide a title, use the title resolver.
    +    $title = isset($main_content['#title']) ? $main_content['#title'] : $this->titleResolver->getTitle($request, $route_match->getRouteObject());
    +
    +    // Determine the title: use the title provided by the main content if any,
    +    // otherwise get it from the routing information.
    +    $options = $request->request->get('dialogOptions', []);
    +
    +    $response->addCommand(new OpenOffCanvasDialogCommand($title, $content, $options));
    +    return $response;
    +  }
    

    A lot of duplication with the parent method but HEAD's ModalRenderer has similar duplication. Perhaps a follow-up issue to clean up both?

tim.plunkett’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

As 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:

Agile process

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.

catch’s picture

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

+++ b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
@@ -0,0 +1,137 @@
+    ];
+    $form['entity_form'] += $this->getEntityForm($this->entity)->buildForm([], $form_state);
+    // Remove the label, ID, description, and buttons from the entity form.
+    unset($form['entity_form']['label'], $form['entity_form']['id'], $form['entity_form']['description'], $form['entity_form']['actions']);
+    // Since the overview form is further nested than expected, update the

This is the bit that looks questionable.

Also while I tried to ignore non-release-managery things, I did notice this:

+++ b/core/modules/outside_in/outside_in.module
@@ -0,0 +1,140 @@
+  if (_outside_in_apply_on_current_page() && $variables['plugin_id'] !== 'system_main_block') {

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.

xjm’s picture

Following up on #176, these are the requirements an experimental module must fulfill:
https://www.drupal.org/core/experimental#requirements

webchick’s picture

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.

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

tim.plunkett’s picture

WRT 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:

    $form = $this->plugin->buildConfigurationForm($form, $form_state);
    unset($form['block_branding']['use_site_name']['#description'], $form['block_branding']['use_site_slogan']['#description']);

As to the cache contexts, I'll take a look at that.

catch’s picture

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

xjm’s picture

#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?

xjm’s picture

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

Bojhan’s picture

Status: Reviewed & tested by the community » Needs work

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

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

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

Moving back to 8.2.x since this is tagged 'beta target'.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
60.14 KB
2.79 KB

Learning cache contexts. Really unsure about what to do in here. Also, maybe _outside_in_apply_on_current_page() should be a service after all?

Status: Needs review » Needs work

The last submitted patch, 187: 2753941-outside_in-187.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
64.63 KB
8.97 KB

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

effulgentsia’s picture

  1. +++ b/core/modules/outside_in/outside_in.module
    @@ -0,0 +1,123 @@
    +  $items['contextual']['#cache']['contexts'][] = 'outside_in_is_applied';
    +  if (\Drupal::service('outside_in.info')->isApplied() && isset($items['contextual']['tab'])) {
    

    I think this can be changed to first check the 'tab' and only apply the cache context if there is.

  2. +++ b/core/modules/outside_in/src/OutsideInInfoInterface.php
    @@ -0,0 +1,18 @@
    +interface OutsideInInfoInterface {
    +
    +  /**
    +   * Determines if the Outside-In logic should be run on the current page.
    +   *
    +   * @return bool
    +   *   TRUE if the Outside-In logic should be run.
    +   */
    +  public function isApplied();
    +
    +}
    

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

webchick’s picture

xjm’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
12.07 KB

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

effulgentsia’s picture

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.

Yeah, 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:

window.sessionStorage.clear();

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.

after logging out and logging back in

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?

and in an entirely different browser

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.

xjm’s picture

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

GrandmaGlassesRopeMan’s picture

FileSize
228.93 KB
210.45 KB

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
65.19 KB
9.34 KB

This ensures the link appears after a cache clear. I never hit that because I was installing via Drush.

Status: Needs review » Needs work

The last submitted patch, 196: 2753941-outside_in-196.patch, failed testing.

xjm’s picture

+++ b/core/modules/outside_in/js/outside_in.js
@@ -0,0 +1,139 @@
+    // Bind a listener to all 'Quick Edit' links for blocks

+++ b/core/modules/outside_in/outside_in.links.contextual.yml
@@ -0,0 +1,4 @@
+  title: 'Quick Edit'

So, 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
65.21 KB
1.85 KB

Agreed with #198, fixed.

Also the data-contextual-id changed due to the last change.

xjm’s picture

+++ b/core/modules/outside_in/outside_in.module
@@ -40,6 +40,18 @@ function outside_in_contextual_links_view_alter(&$element, $items) {
+  // Force a new 'data-contextual-id' attribute on blocks when this module is
+  // enabled so as not to reuse stale data cached client-side.
+  // @todo Remove when https://www.drupal.org/node/2773591 is fixed.

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

xjm’s picture

Oh, 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.

tkoleary’s picture

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,

Can we temporarily add an alert to clear caches in the DSM that appears on module install?

SKAUGHT’s picture

better: clear caches on module enable.

xjm’s picture

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

xjm’s picture

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

  • #180
  • #177
  • #173
  • #171 (kind of broad? but maybe?)
  • #166 / #167
  • #164
  • #144 (also broad but relevant)
  • #109 through #116
  • #103 (totally out of scope but some interesting ideas for the future)
  • #73
  • #30 through #36
  • #24 (need a followup for more broad testing with different themes?)
  • #11 has "Currently the JS targets elements in the Bartik theme. Will need to be updated to be generic" been addressed?
  • #10 and #5 through #7
xjm’s picture

+++ b/core/modules/outside_in/outside_in.module
@@ -0,0 +1,135 @@
+      $output .= '<p>' . t('The Outside-In module is something that we should have help for. For more information, see the <a href=":outside-in-documentation">online documentation for the Outside-In module</a>.', [':outside-in-documentation' => 'https://www.drupal.org/documentation/modules/outside_in']) . '</p>';

LOL.

Yeah, adding that to the required followups in #2762505: Introduce "outside in" quick edit pattern to core. ;)

xjm’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
66.49 KB

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

dawehner’s picture

nod_’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,135 @@
    +  var createOffCanvasWrapper = function (pageWidth) {
    ...
    +  var createTitle = function (title) {
    ...
    +  var createOffCanvasContent = function (data) {
    ...
    +  var createOffCanvasClose = function (offCanvasWrapper, pageWrapper) {
    

    These should be named functions.

    More importantly is there a reason to not use Drupal.theme? that's kinda exactly what it's for.

  2. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,135 @@
    +    return $('<div />', {
    +      'id': 'offcanvas',
    +      'role': 'region',
    +      'aria-labelledby': 'offcanvas-header'
    +    });
    ...
    +    return $('<div />', {class: 'offcanvas-content', html: data});
    

    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.

  3. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,135 @@
    +    return $('<button />', {
    +      'class': 'offcanvasClose',
    +      'aria-label': Drupal.t('Close configuration tray.'),
    +      'html': '<span class="visually-hidden">' + Drupal.t('Close') + '</span>'
    +    }).on('click', function () {
    

    The first part could be html directly but less important.

  4. +++ b/core/modules/outside_in/js/offcanvas.js
    @@ -0,0 +1,135 @@
    +    // Set the initial state of the off-canvas element.
    +    // If the state has been set previously, use it.
    +    Drupal.offCanvas = {
    +      visible: (Drupal.offCanvas ? Drupal.offCanvas.visible : false)
    +    };
    

    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.

  5. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +  // Bind a listener to the 'edit' button
    +  // Toggle the js-outside-edit-mode class on items that we want
    +  // to disable while in edit mode.
    +  $('.contextual-toolbar-tab.toolbar-tab button').on('click', function () {
    +    setToggleActiveMode();
    +  });
    

    Any reason this is not in a behavior?

  6. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +        var parents = $(e.target).parents('.outside-in-editable');
    +        editLink = parents.find('li.outside-inblock-configure a')[0];
    

    -parents +closest

  7. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -0,0 +1,139 @@
    +  var isActiveMode = function () {
    ...
    +  var setToggleActiveMode = function (forceActive) {
    

    named function please

tim.plunkett’s picture

Status: Needs work » Needs review

All of those need to be made into follow-up issues, this is experimental code.

effulgentsia’s picture

  1. +++ b/core/modules/outside_in/outside_in.install
    @@ -0,0 +1,29 @@
    +  // Flush all persistent caches.
    

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

  2. +++ b/core/modules/outside_in/outside_in.install
    @@ -0,0 +1,29 @@
    +  // Add the "use outside-in" permission to all those with "administer block".
    

    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.

effulgentsia’s picture

Issue summary: View changes

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

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

If the known issues is resolved, remaining tasks and blockers - and most issues seem resolved we should be good to go.

DamienMcKenna’s picture

Issue summary: View changes

(minor html fix to issue summary)

GrandmaGlassesRopeMan’s picture

After looking at #210 I've addressed some of the feedback,

  1. Fixed Additionally, we should file a followup issue to update the coding standards to enforce named functions.
  2. I don't agree with this pattern, however I have adjusted each of these to be just HTML
  3. Fixed.
  4. Fixed.
  5. Fixed.
  6. Fixed.
  7. Fixed.
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Thanks. Back to "Needs review" for the interdiff in #216. Also, I'll post another patch shortly for some of the nits in #174.

effulgentsia’s picture

This only fixes points 3 and 5 from #174, since those are trivial. I'll open follow-ups for the rest.

xjm’s picture

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

nod_’s picture

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

webchick’s picture

Issue summary: View changes
FileSize
187.02 KB

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

tedbow’s picture

Ok 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

  1. From cOffCanvasDialogCommand.php
       * @todo Do we need a selector? Or act the same as modal?</li>
        *
  2. . Since we only have 1 element we are ever replacing like modal we don't need the selector parameter.

  3. From OffCanvasTest
        // @todo Add other themes to test against.
        $themes = ['bartik', 'stark'];
    
        // @todo Add RTL Language test for each theme.
    
  4. . 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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I'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!

effulgentsia’s picture

Just fixing some 80 character line wrapping and similar docs nits, so leaving at RTBC.

effulgentsia’s picture

Everything else has a follow-up created or described in the parent issue of #2762505: Introduce "outside in" quick edit pattern to core.

AFAICT, 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.

xjm’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'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!

diff --git a/core/modules/outside_in/js/offcanvas.js b/core/modules/outside_in/js/offcanvas.js
index 9ebd3ea..0f5ae08 100644
--- a/core/modules/outside_in/js/offcanvas.js
+++ b/core/modules/outside_in/js/offcanvas.js
@@ -88,7 +88,6 @@
    *   The Drupal Ajax object.
    * @param {object} response
    *   Object holding the server response.
-   *
    * @param {number} [status]
    *   The HTTP status code.
    */
diff --git a/core/modules/outside_in/outside_in.module b/core/modules/outside_in/outside_in.module
index 905b8f7..d7aa0ce 100644
--- a/core/modules/outside_in/outside_in.module
+++ b/core/modules/outside_in/outside_in.module
@@ -89,7 +89,7 @@ function outside_in_entity_type_build(array &$entity_types) {
 function outside_in_preprocess_block(&$variables) {
   // The main system block does not contain the block contextual links.
   $variables['#cache']['contexts'][] = 'outside_in_is_applied';
-  if (\Drupal::service('outside_in.manager')->isApplicable() && $variables['plugin_id'] !== 'system_main_block') {
+  if ($variables['plugin_id'] !== 'system_main_block' && \Drupal::service('outside_in.manager')->isApplicable()) {
     // Add class to all blocks to allow Javascript to target.
     $variables['attributes']['class'][] = 'outside-in-editable';
   }
@@ -102,7 +102,7 @@ function outside_in_preprocess_block(&$variables) {
  */
 function outside_in_toolbar_alter(&$items) {
   $items['contextual']['#cache']['contexts'][] = 'outside_in_is_applied';
-  if (\Drupal::service('outside_in.manager')->isApplicable() && isset($items['contextual']['tab'])) {
+  if (isset($items['contextual']['tab']) && \Drupal::service('outside_in.manager')->isApplicable()) {
     $items['contextual']['#weight'] = -1000;
     $items['contextual']['#attached']['library'][] = 'outside_in/drupal.outside_in';
 
diff --git a/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
index 73cda59..7bad07f 100644
--- a/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
+++ b/core/modules/outside_in/src/Form/SystemMenuOffCanvasForm.php
@@ -8,7 +8,6 @@
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Plugin\PluginFormBase;
-use Drupal\Core\Render\Element;
 use Drupal\Core\Routing\RedirectDestinationTrait;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
diff --git a/core/modules/outside_in/tests/modules/offcanvas_test/src/Plugin/Block/TestBlock.php b/core/modules/outside_in/tests/modules/offcanvas_test/src/Plugin/Block/TestBlock.php
index da72f9b..efb38e0 100644
--- a/core/modules/outside_in/tests/modules/offcanvas_test/src/Plugin/Block/TestBlock.php
+++ b/core/modules/outside_in/tests/modules/offcanvas_test/src/Plugin/Block/TestBlock.php
@@ -6,7 +6,7 @@
 use Drupal\Core\Url;
 
 /**
- * Provides a 'Powered by Drupal' block.
+ * Provides an 'Off-canvas test block' block.
  *
  * @Block(
  *   id = "offcanvas_links_block",
diff --git a/core/modules/outside_in/tests/src/Unit/OutsideInManagerTest.php b/core/modules/outside_in/tests/src/Unit/OutsideInManagerTest.php
index 53b83e7..b08851f 100644
--- a/core/modules/outside_in/tests/src/Unit/OutsideInManagerTest.php
+++ b/core/modules/outside_in/tests/src/Unit/OutsideInManagerTest.php
@@ -33,6 +33,9 @@ public function testIsApplicable($is_admin_route, $route_name, $has_permission,
     $this->assertSame($expected, $outside_in_manager->isApplicable());
   }
 
+  /**
+   * Data provider for ::testIsApplicable().
+   */
   public function providerTestIsApplicable() {
     $data = [];
 

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?

+++ b/core/modules/outside_in/outside_in.module
@@ -0,0 +1,137 @@
+/**
+ * Implements hook_page_top().
+ */
+function outside_in_page_top(array &$page_top) {
+  // Opens a div for consistent wrapping to {{ page }} render in all themes.
+  $page_top['outside_in_tray_open'] = [
+    '#markup' => '<div id="main-canvas-wrapper"><div id="main-canvas">',
+    '#weight' => 1000,
+  ];
+}
+
+/**
+ * Implements hook_page_bottom().
+ */
+function outside_in_page_bottom(array &$page_bottom) {
+  // Closes a div for consistent wrapping to {{ page }} render in all themes.
+  $page_bottom['outside_in_tray_close'] = [
+    '#markup' => '</div></div>',
+    '#weight' => -1000,
+  ];
+}

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?

  • alexpott committed 42b81c6 on 8.3.x
    Issue #2753941 by tedbow, tim.plunkett, tkoleary, SKAUGHT, drpal,...

  • alexpott committed abdf37c on 8.2.x
    Issue #2753941 by tedbow, tim.plunkett, tkoleary, SKAUGHT, drpal,...
alexpott’s picture

I'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?

tedbow’s picture

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

Which actually is another piece of feedback - should outside_in and block_place be merged?

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)

  1. Update block_place to use data-dialog-type=offcanvas, if outside_in is enabled
  2. Then hopefully #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it, this could happen while the rest of outside_in is still experimental
  3. Then if block_place is merged into part the core block module it will be using a core library, offcanvas, " if outside_in is enabled" won't be needed
  4. Then get outside_in to a stable state

re:

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?

This will change in #2784463: Convert outside_in_page_(top|bottom)() to a #theme_wrappers

webchick’s picture

Gasp! :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

xjm’s picture

Component: toolbar.module » outside_in.module
xjm’s picture

Issue tags: +8.2.0 release notes
tkoleary’s picture

Issue summary: View changes
FileSize
1004.85 KB
naveenvalecha’s picture

Added change record for the Outside-In https://www.drupal.org/node/2786039

SKAUGHT’s picture

#227

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?

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.

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint
tedbow’s picture

Component: outside_in.module » settings_tray.module

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