Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Problem/Motivation

For modules to use the off-canvas dialog provided by the Settings Tray module without the Settings Tray

The Settings Tray module besides what its name suggests provides a lot more that just a tray. It provides a quick way to edit blocks and other configuration and also expands edit mode to disable links and form element. This might not be desired for every site and there should be an easy way to turn this off without losing the ability for other module to use the tray.

Proposed resolution

Move 'off_canvas' dialog renderer from the Settings Tray module to the core dialog system so that can be used in the same way as the regular dialog(provided by core but not used by core) and modal dialog.

Remaining tasks

Agree on this or a better to bring in consistent off-canvas tray experience to core and contrib that doesn't rely on Settings Tray module.

User interface changes

A new off-canvas dialog/tray could be use by core and contrib.

API Additions

No API changes are made. All existing dialog/modal links will remain same, but the following are API additions:
New off_canvas dialog renderer.
Used like:

 $mylink['attributes'] = [
      'class' => ['use-ajax'],
      'data-dialog-type' => 'dialog',
     'data-dialog-renderer' => 'off_canvas',
    ];

To understand dialog renderers generally @see #2844261: Allow dialog links to specify a renderer in addition to a dialog type

Data model changes

None

CommentFileSizeAuthor
#146 2784443-145.patch52.76 KBtedbow
#137 2784443-137.patch52.53 KBtedbow
#137 interdiff-131-137.txt958 bytestedbow
#131 2784443-131.patch52.98 KBtedbow
#130 2784443-130-do-not-test.patch72.49 KBtedbow
#130 interdiff-127-130.txt2.51 KBtedbow
#127 2784443-127.patch51.72 KBtedbow
#125 2784443-125.patch82.24 KBtedbow
#120 2784443-120.patch82.45 KBtedbow
#119 move_off_canvas-2784443-119-copies-example.patch52.17 KBCottser
#118 2784443-118.patch83.25 KBtedbow
#115 2784443-115.patch83.27 KBtedbow
#115 interdiff-112-115.txt6.86 KBtedbow
#112 2784443-111.patch84.97 KBWim Leers
#112 interdiff.txt7.8 KBWim Leers
#110 Screen Shot 2017-08-03 at 15.17.56.png21.41 KBWim Leers
#109 2784443-109.patch82.25 KBWim Leers
#109 interdiff.txt11.41 KBWim Leers
#107 2784443-107.patch75.23 KBtedbow
#107 interdiff-103-107.txt13.72 KBtedbow
#103 2784443-102.patch70.52 KBtedbow
#103 interdiff-100-102.txt984 bytestedbow
#100 2784443-100.patch70.66 KBtedbow
#100 interdiff-95-100.txt1.24 KBtedbow
#95 2784443-95.patch70.14 KBtedbow
#95 interdiff-94-95.txt4.31 KBtedbow
#94 2784443-94.patch72.36 KBtedbow
#94 interdiff-93-94.txt5.6 KBtedbow
#93 2784443-93.patch67.82 KBtedbow
#91 2784443-91.patch18.15 KBtedbow
#91 interdiff-84-91.txt4.88 KBtedbow
#84 2784443-84.patch20.73 KBtedbow
#84 interdiff-81-84.txt1.66 KBtedbow
#81 2784443-81.patch20.4 KBtedbow
#81 interdiff-79-81.txt787 bytestedbow
#79 2784443-79.patch19.98 KBtedbow
#79 interdiff-73-79.txt2.98 KBtedbow
#73 interdiff-70-73.txt724 bytestedbow
#73 2784443-73.patch17.31 KBtedbow
#70 2784443-70.patch17.33 KBtedbow
#70 2784443-70.patch17.33 KBtedbow
#68 2784443-68.patch124.21 KBtedbow
#68 interdiff-65-68.txt4.64 KBtedbow
#65 interdiff-2784443-63-65.txt44.73 KBtedbow
#65 2784443-65.patch125.79 KBtedbow
#63 2784443-63.patch81.07 KBtedbow
#63 interdiff-2784443-56-62.txt663 bytestedbow
#56 2784443-56.patch80.99 KBtedbow
#56 2784443-56-no-css-changes-do-not-test.patch18.17 KBtedbow
#43 interdiff-32-43.txt7.34 KBtedbow
#43 2784443-43.patch70.63 KBtedbow
#32 2784443-32.patch70.54 KBtedbow
#32 interdiff-28-32.txt16.49 KBtedbow
#28 interdiff-24-28.txt40.51 KBtedbow
#28 2784443-28.patch65.08 KBtedbow
#24 2784443-24.patch40.02 KBtedbow
#24 interdiff-22-24.txt10.09 KBtedbow
#22 interdiff-19-22.txt3.75 KBtedbow
#22 2784443-22.patch33.48 KBtedbow
#19 2784443-19.patch29.88 KBtedbow
#14 interdiff-2784443-12-14.txt7.01 KBtedbow
#14 2784443-14.patch25.84 KBtedbow
#12 interdiff-2784443-10-12.txt569 bytestedbow
#12 2784443-12.patch21.04 KBtedbow
#10 2784443-9.patch20.62 KBtedbow
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

effulgentsia created an issue. See original summary.

nod_’s picture

Issue tags: +JavaScript
tedbow’s picture

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

Core dialogs provide this functionality and more. Just need a little glue code for positioning I don't think we need a new lib for this.

tedbow’s picture

@nod_ I like the direction #2786459: "Offcanvas" tray should be using the existing dialog system is going but I still like the idea of this current issue. I think we need to make it as easy as possible to use the "offcanvas" style tray.

. Just need a little glue code for positioning I don't think we need a new lib for this.

I don't that glue code that is being developed in #2786459: "Offcanvas" tray should be using the existing dialog system belongs in the outside_in.module when it is stable.

Core and contrib should be able to use this new tray just by setting 'data-dialog-type' => 'offcanvas', and maybe attaching a library without outside_in enabled. That way contrib doesn't have to provide the glue code each time. We can then have a consistent DX and UX experience for a tray.

nod_’s picture

Like I was saying on IRC yesterday, I don't want a new data-dialog-type type, it's supposed to be following the HTML5 spec and there is only dialog or modal that make sense.

I do agree with the fact that the positioning code shouldn't be in the outsidein module in the end. I think we should extend the dialogs to add some sort of named positioning option. The current default behavior for modals is center-of-displaced-area, this new one would be a displacing-sidebar kinda thing.

Ideally we would add a drupalPosition option to data-dialog-options (just like we added drupalAutoButtons) and name this one sidebar or something.

Wim Leers’s picture

Status: Active » Postponed (maintainer needs more info)
Related issues: +#2786459: "Offcanvas" tray should be using the existing dialog system

#2786459: "Offcanvas" tray should be using the existing dialog system happened. It's not clear what's next here, or if this can be closed. Per #65–#68 in that issue, a follow-up is still needed. Perhaps this can be that follow-up?

tedbow’s picture

Title: Stabilize drupal.offcanvas library and move it into core.libraries.yml » Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Active

@Wim Leers updated title and summary to reflect change for #2786459: "Offcanvas" tray should be using the existing dialog system

tedbow’s picture

Issue summary: View changes
Status: Active » Needs review

Ok. Here is the first try at the patch.

Remaining tasks
1. Move CSS
2. Figure out other remaining tasks ;)

tedbow’s picture

Whoops forgot to attach patch!

Status: Needs review » Needs work

The last submitted patch, 10: 2784443-9.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
21.04 KB
569 bytes

Removing {% spaceless %} from template because this causes tests to fail.

Status: Needs review » Needs work

The last submitted patch, 12: 2784443-12.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
25.84 KB
7.01 KB

Ok this patch

  1. Adds back \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInJavascriptTestBase::enableTheme which I accidently deleted
  2. Fixes fail in \Drupal\KernelTests\Core\Theme\StableTemplateOverrideTest::testStableTemplateOverrides
  3. Moves CSS that relates to offcanvas out of Setting Tray module

Status: Needs review » Needs work

The last submitted patch, 14: 2784443-14.patch, failed testing.

droplet’s picture

Just my little thoughts,

I think it's not stable enough to move to CORE. In experiment module section, it gets committed to Git faster than CORE.

Here're few problems I thought it could be addressed in outside_in module threads first and then sync it back to here:

- Almost all CSS selectors aren't following the CSS Naming conversion (SMACSS)

(Personally, I'm fine with the IDs. But I thought the Drupal banned. Needs a check on CSS standard page and frontend persons)

#offcanvas or .offcanvas should be the main component.
then

#offcanvas-X
#offcanvas-wrapper / #offcanvas__wrapper
#offcanvas__child

It should not double #id #id.

  1. +++ b/core/modules/system/tests/modules/offcanvas_test/src/Controller/TestController.php
    @@ -54,7 +54,7 @@ public function linksDisplay() {
    -            'outside_in/drupal.outside_in',
    
    @@ -69,7 +69,7 @@ public function linksDisplay() {
    -            'outside_in/drupal.outside_in',
    
    @@ -84,7 +84,7 @@ public function linksDisplay() {
    -            'outside_in/drupal.outside_in',
    
    @@ -127,7 +127,7 @@ public function otherDialogLinks() {
    -          'outside_in/drupal.outside_in',
    

    Off-topic:

    I think in other experiment modules, we could remind the developers to prefix the name with "experiment" in between. Therefore, any movement, we just search & replace it instead.
    It's good for Core & Contributed usages.

  2. +++ b/core/themes/stable/templates/layout/offcanvas-page-wrapper.html.twig
    @@ -0,0 +1,21 @@
    +    <div class="offcanvas-lining"></div>
    

    Needed to document what it is

+++ b/core/modules/outside_in/outside_in.module
@@ -38,6 +38,8 @@ function outside_in_contextual_links_view_alter(&$element, $items) {
+      'data-outside-in-edit' => TRUE,
+      //'data-dialog-options' =>

this two lines seems unrelated to this scope

tedbow’s picture

I think it's not stable enough to move to CORE. In experiment module section, it gets committed to Git faster than CORE.

@droplet ok I think you are correct.

Hopefully the patch I made will at least point at needs to be done.

I have just gone through the outside_in.module issue queue and marked every issue that deals only with the offcanvas portion of the module as a child issue of this 1. There are currently 5(might find more). I will try to keep them up to date. Once all the child issues are fixed we can good ahead with this.

I have just update #2815831: Move Off-canvas related CSS from drupal.outside_in library to drupal.off_canvas with some of @droplet's suggestions about css selectors. Lets solve that in that issue.

tedbow’s picture

Update: almost all the child issues have been committed.

#2820135: Allow any contextual link to opt-in to being displayed via offcanvas is RTBC. I just requeued a 8.2x test but hopefully this will be committed soon

I started #2844261: Allow dialog links to specify a renderer in addition to a dialog type for something to work on while the child issues to this were being finished. I think probably I will able to close that and bring that back in here.

#2843901: Settings tray should use toolbar models to close the toolbar items is a 1 line javascript fix that should be committed before this 1(or just incorporated here) but should not stop working on this issue.

tedbow’s picture

Status: Needs work » Needs review
FileSize
29.88 KB

Ok this patch moves the the Offcanvas tray out of Setting module.

Some notes

  1. #2820135: Allow any contextual link to opt-in to being displayed via offcanvas would hopefully be done before this issue but should stop work. We can easily update this patch if that issue is committed
  2. #2844261: Allow dialog links to specify a renderer in addition to a dialog type is included in this patch including the added test \Drupal\Tests\system\FunctionalJavascript\ModalRendererTest. This test that data-dialog-renderer will actually cause a different mainContentRenderer to be used for the dialog link.
  3. \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInJavascriptTestBase could probably go away but I think it makes sense until #2821724: Create Javascript Tests for Contextual Links. But that issue doesn't deal with anything that is moving out of Settings Tray in this issue

Status: Needs review » Needs work

The last submitted patch, 19: 2784443-19.patch, failed testing.

tedbow’s picture

Ok, great all test passed except for those related to stable theme. I need to confirm but I think we need to copy any new library assets into the theme.

Also noticed this issue has not been fixed so marking as child of this #2828912: Offcanvas width is not reset if tray is open with different width and not closed
It is small change but should stop review of this issue.

tedbow’s picture

Status: Needs work » Needs review
FileSize
33.48 KB
3.75 KB

Ok added the new template and the css to the stable theme. That should allow the Stable*Override tests to pass.

Also remove the dependency on outside_in from a test module.

Status: Needs review » Needs work

The last submitted patch, 22: 2784443-22.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
10.09 KB
40.02 KB

Ok this patch does a number of things
I will comment after I update so I can use dreditor.

tedbow’s picture

Notes on previous patch.

  1. +++ b/core/misc/dialog/offcanvas.css
    @@ -1,23 +1,39 @@
    - *
    - * @todo Move CSS into core dialog library https://www.drupal.org/node/2784443.
    

    This the current so the @todo can be removed. Several more in this patch.

  2. +++ b/core/misc/dialog/offcanvas.css
    @@ -1,23 +1,39 @@
    -    padding: 0 20px;
    -    /* Prevent horizontal scrollbar. */
    -    overflow-x: hidden;
    -    overflow-y: auto;
    +  padding: 0 20px;
    +  /* Prevent horizontal scrollbar. */
    +  overflow-x: hidden;
    +  overflow-y: auto;
    

    The CSS was indented 4 spaces instead of 2.

  3. +++ b/core/misc/dialog/offcanvas.css
    @@ -1,23 +1,39 @@
    + * Force the tray to be 100% width at the same breakpoint the dialog system uses
    + * to expand dialog widths.
    + */
    +@media all and (max-width: 48em) {
    +  /* 768px */
    +  .ui-dialog.ui-dialog-offcanvas {
    +    width: 100% !important;
    +  }
    +
    ...
    +  .js-tray-open {
    

    Moved the css added in #2793849: Handle offcanvas differently at lower widths from outside_in.css to offcanvas.css because this behavior should happen without outside_in enabled.

  4. +++ b/core/misc/dialog/offcanvas.motion.css
    @@ -5,24 +5,23 @@
    + * @todo Add a configuration option for browser rendering performance to disable
    + *   this file: https://www.drupal.org/node/2784443.
    

    Left this portion of the @todo referencing this issue. Because I was not clear if we actually need to this.

    Will the transitions cause problems for more complex layouts?

  5. +++ b/core/modules/system/src/Tests/Ajax/OffCanvasDialogTest.php
    @@ -13,13 +13,6 @@
    -   * Modules to enable.
    -   *
    -   * @var array
    -   */
    -  public static $modules = ['outside_in'];
    

    Should not need to enable outside_in for any system tests.

  6. +++ b/core/modules/system/tests/modules/offcanvas_test/src/Controller/TestController.php
    @@ -55,7 +55,7 @@ public function linksDisplay() {
    -            'outside_in/drupal.outside_in',
    +            'core/drupal.ajax',
    

    Update the tests to use the correct library. To get these links to work only drupal.ajax library is needed. \Drupal\Core\Render\MainContent\OffCanvasRender will handle attaching offcanvas library when link is used.

  7. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php
    @@ -5,7 +5,7 @@
    - * @group outside_in
    + * @group system
    

    Just corrected the test group.

tedbow’s picture

There are inconsistencies in the files concerning whether "off canvas" should be considered 2 words.
In php classes the pattern is 2 words like OffCanvasRender.php.
But in css and js 1 word. offcanvas.css and offcanvas.js

I am not sure on the standard for css and js(when not a js class) file names. - vs _ for word splits. I see both in core. I see naming standards for inside js and css files but not the files themselves
If someone can point be to the standards that would be great.
But first look.

  1. +++ b/core/misc/ajax.js
    index 0000000..0e46c6a
    --- /dev/null
    
    --- /dev/null
    +++ b/core/misc/dialog/offcanvas.css
    

    Name should change to off-canvas.css

  2. +++ b/core/misc/dialog/offcanvas.css
    --- a/core/modules/outside_in/js/offcanvas.js
    +++ b/core/misc/dialog/offcanvas.js
    

    change to: off_canvas.js
    I think the standard for js files is underscores but not sure.

  3. +++ b/core/misc/dialog/offcanvas.js
    --- a/core/modules/outside_in/css/offcanvas.motion.css
    +++ b/core/misc/dialog/offcanvas.motion.css
    

    change to: off_canvas.motion.css

martin107’s picture

If someone can point be to the standards that would be great.

This link has the variable naming convention, and the case study is a useful read.

https://www.drupal.org/node/1887918#separate-concerns

As for the css filename convention here is some relevant text from a different document

If a module attaches a CSS file to a template file, the CSS file should be named the same as the template file, e.g. the system-plugin-ui-form.html.twig CSS file should be named system-plugin-ui-form.css

https://www.drupal.org/node/1887922

Anyway I hope that helps... if you still have lingering question ... just let me know and I will dig around some more

tedbow’s picture

@martin107 thanks for the links. This patch does a huge rename of offcanvas to off-canvas or off_canvas depending on the context.

tedbow’s picture

Issue summary: View changes

Updated summary to mention related issues.

Also crossed of tasks already done

tim.plunkett’s picture

Only nitpicks left, the actual code moves are 100% complete.

  1. +++ b/core/lib/Drupal/Core/Ajax/OpenOffCanvasDialogCommand.php
    @@ -1,8 +1,6 @@
      * Defines an AJAX command to open content in a dialog in a off-canvas tray.
    
    +++ b/core/misc/dialog/off-canvas.css
    @@ -0,0 +1,39 @@
    + * CSS for Offcanvas tray.
    

    There are still 14 instances of "offcanvas" or "Offcanvas" in core and 13 of "off canvas".

    96 of "off-canvas" and 46 of "OffCanvas" are fine.

  2. +++ b/core/misc/dialog/off-canvas.css
    @@ -0,0 +1,39 @@
    +/**
    ...
    +/*
    

    Missing an extra *

  3. +++ b/core/misc/dialog/off-canvas.css
    @@ -0,0 +1,39 @@
    + */
    +/* Position the dialog-off-canvas tray container outside the right of the viewport. */
    

    Missing blank line

Note the CSS problems are also in the Stable theme version, need to be fixed twice.

drpal’s picture

Just minor changes to ajax.js which appear reasonable to me.

tedbow’s picture

@tim.plunkett thanks for review!
1. I replace all of these. I also updated a bunch of comments to make referring to the off-canvas dialog consistent.
In various places in the comments it was referring to it as

  • off canvas dialog tray
  • off-canvas tray
  • Off-canvas tray
  • off canvas dialog
  • dialog-off-canvas tray
  • off-canvas tray dialog
  • off canvas
  • off-canvas dialog

So now it always refers as "off-canvas dialog" in the comments.
2,3 Fixed in css, also in stable.

@drpal Thanks for confirmation!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I believe this is ready.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

Thanks everyone!

Let's get a JavaScript subsystem maintainer review on this issue (if available) since it is making a noteworthy JS API addition to stable core.

tedbow’s picture

Removing #2843901: Settings tray should use toolbar models to close the toolbar items as related and in summary because this only happens in the Setttings Tray module outside_in.js which staying in the module. Not the off-canvas dialog

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

It looks good, one question though.

What is the exact public API we're introducing that is bound to our major semver, if any? I'd like to have droplet opinion on this to know if the api we have is sufficient. He had more use of it than me.

tedbow’s picture

Issue summary: View changes

@nod_ thanks for the review.
If updated the "API Changes" portion of the IS

tedbow’s picture

Issue summary: View changes

@tim.plunkett pointed out that these are API additions not changes. All existing dialog links will remain the same, no changes needed to use the existing dialog and modal.
Updated the summary

webchick’s picture

Assigned: Unassigned » droplet

Assigning to droplet for review.

webchick’s picture

Priority: Normal » Major

Also bumping priority, since this is the main blocker to getting this functionality stable.

droplet’s picture

Assigned: droplet » Unassigned

Busy life, can't find time to review big patch. But now I realized I only need to review a very small part in JS core. Setting Tray is still in Experimental module section.

  1. +++ b/core/core.services.yml
    @@ -1059,6 +1059,11 @@ services:
    +      - { name: render.main_content_renderer, format: drupal_dialog.off_canvas }
    
    +++ b/core/misc/ajax.js
    @@ -312,6 +313,8 @@
    +   * @prop {string} [drupalDialogRenderer]
    +   *   Renderer for the dialog, 'off_canvas' supported.
    

    Should we document it better? To tell it will be "drupal_dialog.off_canvas" and defined in PHP..etc.
    (I guessed this is a class name for HTML wrapper at first glance, lol)

  2. +++ b/core/misc/ajax.js
    @@ -64,6 +64,7 @@
    +        element_settings.drupalDialogRenderer = $(this).data('dialog-renderer');
    
    @@ -526,7 +529,11 @@
    +      wrapper += '.' + element_settings.drupalDialogRenderer;
    

    ===

    this condition but easier to read?

        if (element_settings.drupalDialogRenderer) {
          ajax.options.url += '.' + element_settings.drupalDialogRenderer;
        }
  3. +++ b/core/core.services.yml
    @@ -1059,6 +1059,11 @@ services:
    +    class: Drupal\Core\Render\MainContent\OffCanvasRender
    

    OffCanvasRenderER?

  4. +++ b/core/misc/dialog/off-canvas.js
    @@ -12,7 +8,7 @@
    +  // the tray will be %100 width. @see off-canvas.css
    

    what is %100?

tedbow’s picture

@droplet

Busy life, can't find time to review big patch.

Totally understand, thanks for the review.
1. I agreed this could use an expanded comment.

Updated the comment drupalDialogRenderer but first I had to update comment above it about [dialogType]. These 2 are used together. Personally it took a while to figure when I was first looking at the dialog system how dialogType affected selection of renderer php service.
So I updated these 2 comments, even though dialogType wasn't introduced in this issue the description really needed to be expanded because it is necessary to explain how drupalDialogRenderer works.

2. I am not really sure what you are suggesting here. Could you explain?

3. Nice catch updated.
4. Expanded the comment to explain further.

 // The minimum width of the body to use body displace. This value needs to
  // match the body width at which the Off-Canvas dialog will be forced to %100
  // width via CSS . Below this width the body will not shift to show the dialog
  // on the side instead the dialog will cover the entire page.
  // @see off-canvas.css
droplet’s picture

2. I am not really sure what you are suggesting here. Could you explain?

my original thought but now I think we can skip this:

    // Before patch
    ajax.options.url += Drupal.ajax.WRAPPER_FORMAT + '=drupal_' + (element_settings.dialogType || 'ajax');

    // @tedbow patching
    var wrapper = 'drupal_' + (element_settings.dialogType || 'ajax');
    if (element_settings.drupalDialogRenderer) {
      wrapper += '.' + element_settings.drupalDialogRenderer;
    }
    ajax.options.url += Drupal.ajax.WRAPPER_FORMAT + '=' + wrapper;

    // @droplet suggestion
    ajax.options.url += Drupal.ajax.WRAPPER_FORMAT + '=drupal_' + (element_settings.dialogType || 'ajax');
    if (element_settings.drupalDialogRenderer) {
      ajax.options.url += '.' + element_settings.drupalDialogRenderer;
    }

 // The minimum width of the body to use body displace. This value needs to
  // match the body width at which the Off-Canvas dialog will be forced to %100
  // width via CSS . Below this width the body will not shift to show the dialog
  // on the side instead the dialog will cover the entire page.
  // @see off-canvas.css

I got the meaning by reading source code but I still don't understand what is %100 in above comment. Is it equal to 100%?. (Not a native English speaking :) As developer, I first thought that is: minDisplaceWidth % 100 = 68 )

OK. So to my understanding, minDisplaceWidth is the breakpoint to trigger $mainCanvasWrapper padding. It need not to match CSS all time.

  1. +++ b/core/misc/dialog/off-canvas.js
    @@ -102,25 +101,25 @@
    +          .on('dialogContentResize.off-canvas', eventData, debounce(bodyPadding, 100))
    

    Is it redundant?

  2. +++ b/core/misc/dialog/off-canvas.js
    @@ -102,25 +101,25 @@
             $element.dialog('widget').attr('data-offset-' + edge, '');
    

    namespaced with drupal if it's not jQuery UI attr.

    data-drupal-offset

  3. +++ b/core/misc/dialog/off-canvas.js
    @@ -102,25 +101,25 @@
       $(window).on({
    

    Code started from above LINE:

    This is what we coded in other scripts in CORE. Personally, I don't like this code style and I wonder if we want to make the same mistake again. It forced to patching by swapping the whole off-canvas.js. Or dirty coding, adding another listener.

    For example,

        'dialog:beforeclose': function (event, dialog, $element) {
          if ($element.is('#drupal-off-canvas')) {
            $('body').removeClass('js-tray-open');
            $(document).off('.off-canvas');
            $(window).off('.off-canvas');
            $mainCanvasWrapper.css('padding-' + edge, 0);
          }
        }
    

    If we want to keep the padding above. We adding another listener.

    1. This is set to ZERO
    2. This is set back to padding value.

    In a complicated system, it will lose in control.

EDITED:

even minDisplaceWidth can't override.

droplet’s picture

  var edge = document.documentElement.dir === 'rtl' ? 'left' : 'right';

It's good to provide an option for this. Maybe, for some theme, placed at LEFT is better than RIGHT. Not just for RTL.

tkoleary’s picture

It's good to provide an option for this. Maybe, for some theme, placed at LEFT is better than RIGHT. Not just for RTL.

Well, no. Not unless we want to write the logic that will automatically move the *toolbar* vertical tray to the right when settings tray is to the left. Otherwise we could have two trays, toolbar and settings tray, on one side which would be very odd.

The questions we need to be asking are not 'might some theme or module developer want to do this?' but 'what user problem would this solve?'. In this case none that I can see.

SKAUGHT’s picture

#46
i recall some of the early comments about outside_in about the possibility of the tray being on other sides (ie: top & bottom). which as a generic tool, the tray 'would' be possible to be anywhere for other functionality beside related block settings..

ie: a content overview tray at the bottom for Roles who are Editors.

@trays_everywhere. (:

tedbow’s picture

@droplet thanks for your review again in #44
Sorry it took me so long to get back to it I was looking at other related issue.
I am wondering we should postpone this until these issues are resolved:
#2826722: Add a 'fence' around settings tray with aggressive CSS reset.
#2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer
#2853208: [META] Determine best method ensure consistent theming of Off Canvas Tray

We could commit this 1 first but probably will less confusing to get those fixes in first.

But let me address you review so we have a record.

I got the meaning by reading source code but I still don't understand what is %100 in above comment. Is it equal to 100%?. (Not a native English speaking :) As developer, I first thought that is: minDisplaceWidth % 100 = 68 )

Sorry for this it is not your English but my typing :(
It should say "100%"

#44.1 Yes this appears to be redundant. I have commented it out locally and I can't see a change in behaviour. I tried resizing the dialog and resizing the window and it all seems to happen smoothly.
#44.3 I am not really sure about the point on the coding style

Re:

1. This is set to ZERO
2. This is set back to padding value.

In a complicated system, it will lose in control.

Are you talking about this
$mainCanvasWrapper.css('padding-' + edge, 0);
I am not sure what is so complicated about this.
When the dialog is closed the padding that was added so that dialog didn't cover up the body needs to be removed.
If not the padding will remain and there will be a empty space.
Maybe I am missing something.

tkoleary’s picture

@tedbow

I am wondering we should postpone this until these issues are resolved

I think so. I would say the order should be to commit:

  1. #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
  2. #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer
  3. #2858879: Create a library in Seven theme to contain offcanvas styles

And then this issue.

droplet’s picture

Thanks.

and an issue to rename offcanvas to off_canvas? It reduces a huge amount of changes and better patch to review.

tedbow’s picture

Status: Needs review » Postponed

@droplet

and an issue to rename offcanvas to off_canvas? It reduces a huge amount of changes and better patch to review.

Yes that is good idea.

@MaxFire is working on this #2862625: Rename offcanvas to two words in code and comments. at DrupalDevDays in Seville. It is very close.

Postponing on that issue and #2826722: Add a 'fence' around settings tray with aggressive CSS reset.

miro_dietiker’s picture

Status: Postponed » Needs review

@tedbow Why do we need to postpone this issue on the fence one?
It seems to me it's mostly only a CSS thing and doesn't change anything like an API?

tedbow’s picture

Status: Needs review » Postponed

@miro_dietiker is delayed on #2826722: Add a 'fence' around settings tray with aggressive CSS reset. because:

  1. Currently the CSS is not cleaning divide between what is needed for the Settings Tray module and what is need for the actual dialog. That issue will resolve that.
  2. In several UX meetings the consensus was the tray should have admin feel.
  3. If we move the tray to the dialog system without the CSS somewhat stabilized then people will start to use the tray. If they want it to look adminy they will the start applying there styles modules using the tray or custom theme.
  4. If we later move the CSS reset to the dialog system it will invariably conflict with the CSS people already applied.

@DyanneNova was working the CSS reset I will check with her to see how close she is to finishing it.

tedbow’s picture

tedbow’s picture

Status: Postponed » Needs review
FileSize
18.17 KB
80.99 KB

Ok #2826722: Add a 'fence' around settings tray with aggressive CSS reset. is mostly staying at RTBC so I think it is time to start on this.

Since the last patch in #43 2 important issues have landed

  1. #2862625: Rename offcanvas to two words in code and comments. these changes were polluting this patch before
  2. #2844261: Allow dialog links to specify a renderer in addition to a dialog type had all necessary changes to ajax.es6.js. And also tests for data-dialog-renderer.

This should make this patch much cleaner.

I will CSS changes from #2826722: Add a 'fence' around settings tray with aggressive CSS reset. in this patch for now. I will also include a "no-css" patch without the changes. This patch will not work but should be easier to review. The no CSS patch is 60k smaller.

Once #2826722 lands we should just have to apply this patch and move off-canvas.*css files from outside_in/css to core/misc/dialog/css.

@todo Include changes to need to /core/themes
@see #43 to get an idea of what that entails

xjm’s picture

Since this will be adding a new API to stable core, we should probably also draft a change record that briefly explains what the library is and how to use it.

xjm’s picture

Issue tags: +Needs change record
tedbow’s picture

Here is the draft change record: https://www.drupal.org/node/2896596

Status: Needs review » Needs work

The last submitted patch, 56: 2784443-56.patch, failed testing. View results

Wim Leers’s picture

  1. +++ b/core/core.libraries.yml
    @@ -903,3 +903,30 @@ underscore:
    +      misc/dialog/css/off-canvas.theme.css: { group: 200 }
    

    This can't be right?

  2. +++ b/core/lib/Drupal/Core/Ajax/OpenOffCanvasDialogCommand.php
    @@ -47,6 +45,9 @@ public function __construct($title, $content, array $dialog_options = [], $setti
    +    if (empty($dialog_options['dialogClass'])) {
    +      $this->dialogOptions['dialogClass'] = 'ui-dialog-off-canvas';
    +    }
    
    +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -235,7 +235,6 @@
    -          settings.dialogClass += ' ui-dialog-outside-in';
    

    Ok so this is moved from Settings Tray's JS.

  3. +++ b/core/modules/outside_in/outside_in.libraries.yml
    @@ -6,10 +6,7 @@ drupal.outside_in:
    +      css/outside_in.toolbar.css: {}
    

    So all CSS is moved, except the one for the toolbar. That sounds sensible.

    However, shouldn't this depend on the new drupal.dialog.off_canvas library then?

  4. +++ b/core/modules/outside_in/outside_in.module
    @@ -67,26 +67,6 @@ function outside_in_block_view_alter(array &$build) {
    -function outside_in_element_info_alter(&$type) {
    ...
    -function outside_in_theme() {
    
    +++ b/core/modules/system/system.module
    @@ -268,6 +268,9 @@ function system_theme() {
    +    'off_canvas_page_wrapper' => [
    +      'variables' => ['children' => NULL],
    +    ],
    
    @@ -1489,3 +1492,13 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
    +function system_element_info_alter(&$type) {
    

    Why are we moving this into core? I already questioned the need for this at #2894358-3: Front-end Review of Settings Tray module for Stablization. I thought this was only necessary for the Settings Tray JS functionality?

    Moving this from Settings Tray into "core core" means that we at the very least need very clear documentation about why this is necessary.

  5. +++ b/core/modules/system/system.module
    index 8c6cc80242..9680b1842d 100644
    --- a/core/modules/outside_in/tests/modules/off_canvas_test/off_canvas_test.info.yml
    
    --- a/core/modules/outside_in/tests/modules/off_canvas_test/off_canvas_test.info.yml
    +++ b/core/modules/system/tests/modules/off_canvas_test/off_canvas_test.info.yml
    
    +++ b/core/modules/system/tests/modules/off_canvas_test/off_canvas_test.info.yml
    +++ b/core/modules/system/tests/modules/off_canvas_test/off_canvas_test.info.yml
    @@ -4,6 +4,3 @@ description: 'Provides off-canvas test links.'
    
    @@ -4,6 +4,3 @@ description: 'Provides off-canvas test links.'
     package: Testing
     version: VERSION
     core: 8.x
    -dependencies:
    -  - block
    -  - outside_in
    

    Oh, nicely done, looks like this was nicely planned long before this issue could happen :)

  6. +++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTestBase.php
    @@ -1,13 +1,13 @@
      * Base class contains common test functionality for the Settings Tray module.
    

    This comment now needs to be updated.

Wim Leers’s picture

tedbow’s picture

Status: Needs work » Needs review
FileSize
663 bytes
81.07 KB

@Wim Leers thanks for the review!

1. Copied comment to #2826722-120: Add a 'fence' around settings tray with aggressive CSS reset.
2. Yes, since we decide not to do #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer we can always apply this class. Change made in #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
3. Yes you are correct it should depend on drupal.dialog.off_canvas
After #2826722: Add a 'fence' around settings tray with aggressive CSS reset. is committed we can update that
4. It is required see #2896960: Updated inline docs for data-off-canvas-main-canvas in twig template
5. Yep you could always use the tray if the user didn't have access to Settings Tray other functionality so we needed test for it by itself.
6. Updated

Wim Leers’s picture

Status: Needs review » Needs work

Regarding #61.1/#63.1/#2826722-120: Add a 'fence' around settings tray with aggressive CSS reset.:

It's documented in outside_in_css_alter() and outside_in.libraries.yml — #2784443 is just losing that comment.

Which made me realize I forgot something in my review above:

+++ b/core/modules/outside_in/outside_in.libraries.yml
@@ -6,10 +6,7 @@ drupal.outside_in:
       # @todo Set the group higher than CSS_AGGREGATE_THEME so that it overrides
       #   both jQuery UI and Classy's dialog.css, remove in

We still have this an outside_in_css_alter() — do we really need to? Because that means maintaining this hack in multiple places.

tedbow’s picture

Status: Needs work » Needs review
FileSize
125.79 KB
44.73 KB

Copied CSS to stable theme and updated stable.info with library override

Status: Needs review » Needs work

The last submitted patch, 65: 2784443-65.patch, failed testing. View results

Wim Leers’s picture

Title: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it » [PP-1] Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

Since this is blocked by #2826722, I posted #2826722-126: Add a 'fence' around settings tray with aggressive CSS reset. to explain that that is indeed a blocker, and bumped it to major.

We can keep rolling patches here, but at least updating the title to make it clear that this needs another patch to land first.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
124.21 KB

Ok hopefully this will fix 2 failing tests.

The CSS need to be moved up 1 directory so that in stable there wasn't /css/.../css/off-canvas*.css
No other CSS in core/misc have a sub "css" folder anyways so that should be removed.

Needed to copy the template file to stable also.

Status: Needs review » Needs work

The last submitted patch, 68: 2784443-68.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.33 KB
17.33 KB

This patch takes different approach. It does not include any changes #2826722: Add a 'fence' around settings tray with aggressive CSS reset.

So it adds the Off-canvas dialog from the Settings Tray module but does not do a CSS reset.
So if you open the dialog with the links provide by the test module or any other use of the off-canvas dialog except Settings Tray. To test this you can enable the test module off_canvas_test and visit /off-canvas-test-links

If you use the Settings Tray "Quick Edit" block forms they will have current CSS which makes them dark themed.

This is much simpler patch to review hopefully.

tedbow’s picture

whoops uploaded the file twice 😰

tim.plunkett’s picture

  1. +++ b/core/modules/system/system.module
    @@ -1515,3 +1518,13 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
    +  if (isset($type['page'])) {
    +    $type['page']['#theme_wrappers']['off_canvas_page_wrapper'] = ['#weight' => -1000];
    +  }
    

    Shame this is still needed. Would have been nice just to change \Drupal\Core\Render\Element\Page directly, but it can't depend on things in system.module. Hmm...

  2. +++ b/core/modules/system/tests/src/Unit/Ajax/OpenOffCanvasDialogCommandTest.php
    @@ -1,13 +1,13 @@
    +namespace Drupal\Tests\system\Unit\Ajax;
    ...
    + * @coversDefaultClass \Drupal\Core\Ajax\OpenOffCanvasDialogCommand
    

    Would this make more sense to live in \Drupal\Tests\Core\Ajax, as it isn't testing a system.module thing?

  3. +++ b/core/modules/system/tests/src/Unit/Ajax/OpenOffCanvasDialogCommandTest.php
    --- /dev/null
    +++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
    

    This isn't the first time I've wanted a Drupal\Core subsystem to be able to provide a template :(

tedbow’s picture

@tim.plunkett thanks for review.
1. Yes assuming because 3 below we can't change this
2. Yes moved
3. Not sure if that is possible. Not done I guess

drpal’s picture

+++ b/core/lib/Drupal/Core/Render/MainContent/OffCanvasRenderer.php
@@ -47,7 +46,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
diff --git a/core/modules/outside_in/js/off-canvas.es6.js b/core/misc/dialog/off-canvas.es6.js

diff --git a/core/modules/outside_in/js/off-canvas.es6.js b/core/misc/dialog/off-canvas.es6.js
similarity index 100%

similarity index 100%
rename from core/modules/outside_in/js/off-canvas.es6.js

rename from core/modules/outside_in/js/off-canvas.es6.js
rename to core/misc/dialog/off-canvas.es6.js

rename to core/misc/dialog/off-canvas.es6.js
diff --git a/core/modules/outside_in/js/off-canvas.js b/core/misc/dialog/off-canvas.js

diff --git a/core/modules/outside_in/js/off-canvas.js b/core/misc/dialog/off-canvas.js
similarity index 98%

similarity index 98%
rename from core/modules/outside_in/js/off-canvas.js

rename from core/modules/outside_in/js/off-canvas.js
rename to core/misc/dialog/off-canvas.js

Just a move of the off-canvas JavaScript. 🎉

The last submitted patch, 70: 2784443-70.patch, failed testing. View results

tim.plunkett’s picture

Title: [PP-1] Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it » Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
Issue tags: -Needs subsystem maintainer review +Needs issue summary update

This code is RTBC in my opinion. Removing the subsystem review tag from #34 after the review in #74.

This still needs a CR and an IS update now that it is not blocked on the fence CSS issue.

xjm’s picture

It'd also be good for the IS update to include the pros and cons of doing this before the fence issue, so the frontend framework managers can take that into consideration. Thanks!

tedbow’s picture

Issue summary: View changes

Update the summary
Removed old parts dealing with #2844261: Allow dialog links to specify a renderer in addition to a dialog type
which was split into its own issue committed.

Looking to see if we actually need to bring in some CSS dealing with animation of the main canvas resize.

tedbow’s picture

So this patch adds the off-canvas.motion.css which provides the CSS animation for resizing the main canvas when the off-canvas dialog is open or resize.
The version of the off-canvas.motion.css file used in this patch is from the latest clean patch #2826722-112: Add a 'fence' around settings tray with aggressive CSS reset.

This file is currently in the outside_in module but it is not actually loaded in the library.
@see #2856441: Off-canvas CSS files not included in libraries file but we marked that as duplicate where solving in #2826722-92: Add a 'fence' around settings tray with aggressive CSS reset.

So the this now only contains CSS necessary to animate the resizing of the main-canvas smoothly. All the of the other CSS in the reset has been left out of this patch.

The off-canvas dialog in this patch is fully functional but will just take on the styles of the currently active theme.

Status: Needs review » Needs work

The last submitted patch, 79: 2784443-79.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
787 bytes
20.4 KB

#79 was strange test failure. Retesting.

But also since I just added the CSS animation wait it could be that the animation timing is messing up the test.
So I added wait() that is slightly longer than the animation.
This fixed it locally for me.

Not sure why we are not seeing this failure on #2826722: Add a 'fence' around settings tray with aggressive CSS reset.

The last submitted patch, 79: 2784443-79.patch, failed testing. View results

Wim Leers’s picture

now that it is not blocked on the fence CSS issue.

Why is it no longer blocked on the fence CSS issue? My understanding is that this doesn't make sense without the fence CSS issue, because any module using the API/capabilities that this patch introduces would have to do its own CSS overrides?

tedbow’s picture

+++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
@@ -88,7 +88,7 @@ public function linksDisplay() {
-            'outside_in/drupal.outside_in',

We don't actually need to attach the core/drupal.dialog.off_canvas library we can just attach the core/drupal.dialog.ajax library and only attach it once.

\Drupal\Core\Render\MainContent\OffCanvasRenderer will handle attaching core/drupal.dialog.off_canvas. I think this better example for the test because the developer making a link should not have to worry about which library they need to attach for each dialog renderer. Just use the library for ajax libraries and the correct other libraries will be attached in the response.

Re #83
I think it should still wait on the https://www.drupal.org/node/2826722 but I think this patch can be rerolled when it is finished.
Right it is much simpler to understand what is going on without those changes.

My understanding is that this doesn't make sense without the fence CSS issue, because any module using the API/capabilities that this patch introduces would have to do its own CSS overrides?

I do think that this patch could be committed on its own. If we found a way to mitigate this problem because this patch itself does give a fully functional dialog in the same way as our current dialogs which also don't have a CSS reset. But haven't time to think more on that.

Status: Needs review » Needs work

The last submitted patch, 84: 2784443-84.patch, failed testing. View results

tedbow’s picture

Ok there is really weird testing stuff going on here 😱

#81 first did not fail
I retested #81 failed because of unrelated NodeTypeTranslationTest failure

#84 failed because of OutsideInBlockFormTest::testBlocks twice
which is weird because the only change between #81 and #84 is a change to \Drupal\off_canvas_test\Controller\TestController::linksDisplay() (I doubled check this)
which should have absolutely no effect on OutsideInBlockFormTest::testBlocks()

Why on Why 😢

Wim Leers’s picture

At least a dozen RTBC issues failed in the past 24 hours, due to two commits that have since been reverted. This was likely also a victim!

tedbow’s picture

Status: Needs work » Needs review

Setting to needs review for last changes

tedbow’s picture

tim.plunkett’s picture

Issue tags: +Outside-in, +JavaScript

Restoring the other tags.

  1. +++ b/core/misc/dialog/off-canvas.motion.css
    @@ -0,0 +1,10 @@
    +/**
    + * @file
    + * Motion effects for off-canvas dialog.
    + */
    +
    +.dialog-off-canvas__main-canvas {
    +  -webkit-transition: all .7s ease;
    +  -moz-transition: all .7s ease;
    +  transition: all .7s ease;
    +}
    
    +++ /dev/null
    @@ -1,32 +0,0 @@
    -/**
    - * @file
    - * Motion effects for off-canvas dialog.
    - *
    - * Motion effects are in a separate file so that they can be easily turned off
    - * to improve performance if desired.
    - *
    - * @todo Move motion effects file into a core Off-Canvas library and add a
    - *   configuration option for browser rendering performance to disable this
    - *   file: https://www.drupal.org/node/2784443.
    - */
    -
    -/* Transition the off-canvas dialog container, with 2s delay to match main canvas speed. */
    -.ui-dialog-off-canvas .ui-dialog-content {
    -  -webkit-transition: all .7s ease 2s;
    -  -moz-transition: all .7s ease 2s;
    -  transition: all .7s ease 2s;
    -}
    -
    -@media (max-width: 700px) {
    -  .ui-dialog-off-canvas .ui-dialog-content {
    -    -webkit-transition: all .7s ease;
    -    -moz-transition: all .7s ease;
    -    transition: all .7s ease;
    -  }
    -}
    -
    -.dialog-off-canvas__main-canvas {
    -  -webkit-transition: all .7s ease;
    -  -moz-transition: all .7s ease;
    -  transition: all .7s ease;
    -}
    

    Why are these files so different? Sorry if that was discussed already

  2. +++ b/core/modules/system/src/Tests/Ajax/OffCanvasDialogTest.php
    @@ -46,6 +38,7 @@ public function testDialog() {
    +          'dialogClass' => 'ui-dialog-off-canvas',
    

    It's strange to see these additions without corresponding removals. @tedbow tells me they were added via JS before, that part should be removed.

tedbow’s picture

#90.1 This is because I took the file from this issue #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
We should wait until that issue committed until we bring that file in so removing it and references to it and the copy in stable theme.

#90.2 Yes this class was added in off-canvas.es6.js but no longer needs to be. Fixed

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

That makes sense, thanks.
This looks great!

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
67.82 KB

Re-rolling after https://www.drupal.org/node/2826722

This patch also moves all the CSS that deals with the off-canvas dialog for Settings Tray module into /core/misc/dialog

Then adds these overrides to stable theme.

2 other things
Removes modules not necessary for OffCanvasTest from the test $modules var
Added the animation wait for off-canvas dialgo to \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::waitForOffCanvasToOpen

tedbow’s picture

Ok a few clean ups for testing.

  1. Added 'seven' to \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::getTestThemes(). This was not added the file was OutsideInJavascriptTestBase because Seven theme removes all contextual links so Edit Mode would not work. Now that it is in the system module and testing core/ functionality it should not anything about Settings Tray(outside_in)
  2. Because of ⬆️ removed override getTestThemes() in OffCanvasTest
  3. Added override \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::getTestThemes() to remove 'seven' theme.
  4. Moved clickContextualLink() and toggleContextualTriggerVisibility() from OffCanvasTestBase to OutsideInBlockFormTest because this has to do with Settings Tray.
  5. Added a todo in \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::waitForNoElement to remove in #2892440: Provide helper test method to wait for an element to be removed from the page
tedbow’s picture

A couple fixes from #93 reroll.

  • Remove a .orig file and .rej file I accidentally committed to my branch from my patch command
  • Fixed stable.info reference to the wrong folder for the css files.

The last submitted patch, 93: 2784443-93.patch, failed testing. View results

The last submitted patch, 94: 2784443-94.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great

Cottser’s picture

  1. +++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
    @@ -0,0 +1,26 @@
    + *
    + * @ingroup themeable
    

    Minor: Templates in themes (not module-provided) shouldn't have @ingroup themeable per https://www.drupal.org/node/1354#templates.

  2. +++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
    @@ -0,0 +1,26 @@
    +  <div class="dialog-off-canvas__main-canvas" data-off-canvas-main-canvas >
    

    Super minor, pre-existing condition (it's like this in the current template), shouldn't hold up commit. But there's an extra bit of whitespace at the end of this tag. Can be fixed in a tiny followup issue, maybe with similar things.

This is looking good, I can't do a very in-depth review at the moment but there have already been a lot of good reviews :)

tedbow’s picture

@Cottser thanks for taking a look! Fixed both issues in #99

Cottser’s picture

Thanks @tedbow!

+++ b/core/modules/system/templates/off-canvas-page-wrapper.html.twig
@@ -15,12 +15,10 @@
- *
- * @ingroup themeable

The @ingroup themeable should stay in this template, but be removed from the stable template.

For a bit more context, the themeable group lists all the templates provided and should only have the "originals": https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21....

In HEAD (and 8.3.x) there are templates not following this convention but yeah :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 100: 2784443-100.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
984 bytes
70.52 KB

@Cottser yep sorry, that was my intention. Most have bee confused about which file I was editing.

Fixed

UPDATED: Setting back to RTBC because it was set to needs work because I aborted the tests in #100 to remove it from the testing queue since we have a new patch.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Cottser’s picture

👍

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/core.libraries.yml
    @@ -903,3 +903,29 @@ underscore:
    +      misc/dialog/off-canvas.theme.css: { group: 200 }
    

    Still missing the docs for this group thing. Otherwise we make core less maintainable.

  2. +++ b/core/core.services.yml
    similarity index 97%
    rename from core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
    
    rename from core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
    rename to core/lib/Drupal/Core/Ajax/OpenOffCanvasDialogCommand.php
    
    +++ b/core/lib/Drupal/Core/Ajax/OpenOffCanvasDialogCommand.php
    similarity index 89%
    rename from core/modules/outside_in/src/Render/MainContent/OffCanvasRenderer.php
    
    rename from core/modules/outside_in/src/Render/MainContent/OffCanvasRenderer.php
    rename to core/lib/Drupal/Core/Render/MainContent/OffCanvasRenderer.php
    

    Does this move mean these classes should no longer be @internal?

  3. +++ b/core/modules/outside_in/outside_in.module
    diff --git a/core/modules/outside_in/outside_in.services.yml b/core/modules/outside_in/outside_in.services.yml
    deleted file mode 100644
    

    Nice!

  4. +++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
    @@ -53,11 +53,6 @@ public function linksDisplay() {
    -        '#attached' => [
    -          'library' => [
    -            'outside_in/drupal.outside_in',
    -          ],
    -        ],
    
    @@ -71,11 +66,6 @@ public function linksDisplay() {
    -        '#attached' => [
    -          'library' => [
    -            'outside_in/drupal.outside_in',
    -          ],
    -        ],
    

    Why can we remove these?

  5. +++ b/core/modules/system/tests/modules/off_canvas_test/src/Controller/TestController.php
    @@ -86,10 +76,10 @@ public function linksDisplay() {
    +      '#attached' => [
    +        'library' => [
    +          'core/drupal.dialog.ajax',
             ],
           ],
    

    Why do we need to add this?

  6. +++ b/core/themes/stable/css/core/dialog/off-canvas.base.css
    @@ -0,0 +1,238 @@
    + * Set base styles for the settings tray.
    

    These are styles not for Settings Tray, but for the Off-canvas dialog.

  7. +++ b/core/themes/stable/css/core/dialog/off-canvas.button.css
    @@ -0,0 +1,117 @@
    + * Visual styling for buttons in the off canvas tray.
    

    "off canvas tray" -> "off-canvas dialog"

  8. +++ b/core/themes/stable/css/core/dialog/off-canvas.button.css
    @@ -0,0 +1,117 @@
    + * See seven/css/components/buttons.css
    

    s/See/@see/

    Not entirely sure though.

  9. +++ b/core/themes/stable/css/core/dialog/off-canvas.css
    @@ -0,0 +1,57 @@
    + * @todo Move CSS into core dialog library https://www.drupal.org/node/2784443.
    

    This is that issue. That means this todo should be removed.

  10. +++ b/core/themes/stable/css/core/dialog/off-canvas.css
    @@ -0,0 +1,57 @@
    +/* Position the dialog-off-canvas tray container outside the right of the viewport. */
    ...
    +/* Wrap the form that's inside the dialog-off-canvas tray. */
    

    s/tray/dialog/

  11. +++ b/core/themes/stable/css/core/dialog/off-canvas.details.css
    @@ -0,0 +1,61 @@
    + * Visual styling for summary and details in the Settings Tray module's off canvas tray.
    
    +++ b/core/themes/stable/css/core/dialog/off-canvas.dropbutton.css
    @@ -0,0 +1,291 @@
    + * Styles for dropbuttons in the off-canvas tray.
    
    +++ b/core/themes/stable/css/core/dialog/off-canvas.form.css
    @@ -0,0 +1,137 @@
    + * Visual styling for forms in the Settings Tray module's off canvas tray.
    

    More of this.

  12. +++ b/core/themes/stable/css/core/dialog/off-canvas.motion.css
    @@ -0,0 +1,17 @@
    + * @todo Move motion effects file into a core Off-Canvas library and add a
    + *   configuration option for browser rendering performance to disable this
    + *   file: https://www.drupal.org/node/2784443.
    

    Again a todo that's being solved in this very patch.

  13. +++ b/core/themes/stable/css/core/dialog/off-canvas.table.css
    @@ -0,0 +1,90 @@
    + * Visual styling for tables in the Settings Tray module's off canvas tray.
    
    +++ b/core/themes/stable/css/core/dialog/off-canvas.tabledrag.css
    @@ -0,0 +1,122 @@
    + * Table drag behavior for Settings Tray module.
    
    +++ b/core/themes/stable/css/core/dialog/off-canvas.theme.css
    @@ -0,0 +1,89 @@
    +/* Style the tray header. */
    

    More still.

  14. +++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
    @@ -0,0 +1,24 @@
    + * This is used by the outside_in/drupal.off_canvas library to select the
    

    The library name is not updated.

  15. +++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
    @@ -0,0 +1,24 @@
    + * the "off canvas" tray so that no portion of the "main canvas" is obstructed
    

    s/tray/dialog/

tedbow’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
13.72 KB
75.23 KB

@Wim Leers thanks for the thorough review as always!
1. Added comment to explain
2. Remove @internal for OpenOffCanvasDialogCommand. This is the only 1 that other could should be calling directly. If returning an AjaxResponse then a module might want to add a OpenOffCanvasDialogCommand command to open the off-canvas on return.
3. Yes!
4, At some point for ajax dialog links to work with data-dialog-renderer we needed outside_in.js in this library that was actually changed in #2844261: Allow dialog links to specify a renderer in addition to a dialog type so right now not positive if it was needed after that issue.
5. Now for link to work with

'data-dialog-type' => 'dialog',
'data-dialog-renderer' => 'off_canvas',

You don't actually have to attach core/drupal.dialog.off_canvas. You only need to attach core/drupal.dialog.ajax. This allows the generic data-dialog-renderer logic to work. Then OffCanvasRenderer will be selected as the content render service. OffCanvasRenderer will then attach core/drupal.dialog.off_canvas.

So developers only have to know about setting'data-dialog-renderer' => 'off_canvas', They don't have to know which core library supports this renderer, since now no extra module needs to be enabled.
6. Updated comment
7. fixed
8. fixed
9. I true comment has never been stated. Fixed
10. Replaced all with "off-canvas dialog"
12. Removed these todos, made sure the string "2784443" is nowhere in core.
13. replaced
14. updated library name. Made sure the string "outside_in/drupal.off_canvas" is nowhere in core
16. fixed

Wim Leers’s picture

Status: Needs review » Needs work

#107.4: okay, so #2844261: Allow dialog links to specify a renderer in addition to a dialog type should in theory have already done this. No problem then.
#107.5: makes sense. Especially because outside_in/drupal.off_canvas (now core/drupal.dialog.off_canvas) already depends on it. Thanks for the explanation!
#107.12: 👍 for diligence
#107.14: 👍 for diligence


I wanted to RTBC, but grepping for "tray" still yields several results:

+++ b/core/misc/dialog/off-canvas.css
@@ -0,0 +1,55 @@
+ * Force the tray to be 100% width at the same breakpoint the dialog system uses
...
+  /* When tray is at 100% width stop the body from scrolling */
+  .js-tray-open {

+++ b/core/misc/dialog/off-canvas.details.css
@@ -1,6 +1,6 @@
+ * Visual styling for summary and details in the Settings Tray module's off-canvas dialog.

+++ b/core/misc/dialog/off-canvas.form.css
@@ -0,0 +1,137 @@
+ * Visual styling for forms in the Settings Tray module's off-canvas dialog.

+++ b/core/misc/dialog/off-canvas.table.css
@@ -0,0 +1,90 @@
+ * Visual styling for tables in the Settings Tray module's off-canvas dialog.

+++ b/core/modules/system/templates/off-canvas-page-wrapper.html.twig
@@ -5,10 +5,10 @@
  * "main canvas" page element as opposed to the "off canvas" which is the tray
...
  * by the tray. The tray can vary in width when opened and can be resized by the

+++ b/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTestBase.php
@@ -41,6 +41,9 @@ protected function enableTheme($theme) {
+    // Wait just slightly longer than the tray CSS animation.
...
@@ -70,6 +73,8 @@ protected function getTray() {

+++ b/core/themes/stable/css/core/dialog/off-canvas.details.css
@@ -0,0 +1,61 @@
+ * Visual styling for summary and details in the Settings Tray module's off-canvas dialog.

+++ b/core/themes/stable/css/core/dialog/off-canvas.form.css
@@ -1,6 +1,6 @@
+ * Visual styling for forms in the Settings Tray module's off-canvas dialog.

+++ b/core/themes/stable/css/core/dialog/off-canvas.table.css
@@ -1,6 +1,6 @@
+ * Visual styling for tables in the Settings Tray module's off-canvas dialog.

+++ b/core/themes/stable/templates/content/off-canvas-page-wrapper.html.twig
@@ -0,0 +1,24 @@
+ * "main canvas" page element as opposed to the "off canvas" which is the tray
...
+ * by the tray. The tray can vary in width when opened and can be resized by the

.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
11.41 KB
82.25 KB

Addressed my own remarks in #108 (Ted is on vacation).

Wim Leers’s picture

Status: Needs review » Needs work
FileSize
21.41 KB

I figured I'd do some manual testing to ensure it all still works correctly.

I quickly noticed that some icons were missing:

Root cause:

+#drupal-off-canvas .messages--status {
+  background-color: #f3faef;
+  background-image: url(../../../misc/icons/73b355/check.svg);
+  color: #325e1c;
+}
+#drupal-off-canvas .messages--warning {
+  background-color: #fdf8ed;
+  background-image: url(../../../misc/icons/e29700/warning.svg);
+  color: #734c00;
+}
+
+#drupal-off-canvas .messages--error {
+  background-color: #fcf4f2;
+  background-image: url(../../../misc/icons/e32700/error.svg);
+  color: #a51b00;
+}

This is in core/misc/dialog/off-canvas.base.css. The URL resolves to /misc/icons/ffffff/pencil.svg, not /core/misc/icons/ffffff/pencil.svg.

CSS files in this patch are:

  1. being moved from core/modules/outside_in/css/*.css to core/themes/stable/css/core/dialog/*.css
  2. being copied to core/misc/dialog/*.css

In case 1, the depth relative to the root changes from 5 to 6.

In case 2, the depth relative to the root changes from 5 to 4.

This means that in both cases, we need to change the url(…) statements, but that didn't happen for either one. Working to fix that…

droplet’s picture

I'm not against the changes but can we make a quick patch in outside_in module first?

+++ b/core/misc/dialog/off-canvas.es6.js
@@ -2,16 +2,12 @@
-  // the tray will be %100 width. @see outside_in.module.css
+  // the off-canvas dialog will be 100% width. @see core/misc/dialog/off-canvas.css

@@ -136,14 +132,13 @@
-            $('body').addClass('js-tray-open');
+            $('body').addClass('js-off-canvas-dialog-open');

for example these changes. I expect this patch as simple as move around the file names only.

Less or more, I think we have diff and more loose code standard in experimental modules. for example:

    let modalHeight;

    // Let scroll element take all the height available.
    $element.css({ height: 'auto' });
    modalHeight = $widget.height();

We preferred const modalHeight in this case.

Jquery UI

jQuery UI

and many comments changes..etc

and I'd say this issue is a blocker: #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways

Don't want to see a new technical debt soon :)

  1. +++ b/core/themes/stable/css/core/dialog/off-canvas.reset.css
    @@ -0,0 +1,386 @@
    +/* Bootstrap and other frameworks add color to selection. */
    

    Move into CORE, I think we need more general code comment. eg.:

    standardized off-canvas selection color.

  2. +++ b/core/themes/stable/css/core/dialog/off-canvas.table.css
    @@ -0,0 +1,90 @@
    +#drupal-off-canvas table * {
    font-family: "Lucida Grande", 'Lucida Sans Unicode', 'liberation sans', sans-serif;
    +}
    

    I think we need to doc why it's not same as CORE. Or it's a bug..

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.8 KB
84.97 KB

9 lines url(…) statements changed in core/misc/dialog, 9 changed in core/themes/stable/css/core/dialog.

I think this is now RTBC'able again.

Wim Leers’s picture

for example these changes. I expect this patch as simple as move around the file names only.

and many comments changes..etc

The name & comments made sense until this issue. They make sense to change here. (But yes, it'd have been even better if that had already happened.)

We preferred const modalHeight in this case.

Jquery UI

These are both pretty extreme nitpicks which can easily be follow-ups.

and I'd say this issue is a blocker: #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways

Hm, having read that issue, I'm inclined to agree. OTOH, that could also quite easily be a follow-up, because it'd only be refactoring internals. I agree though that significant changes like this should happen before adding new stable functionality to core…

droplet’s picture

tedbow’s picture

@Wim Leers and @droplet thanks for pushing forward!

The patch in #109 should have also update the CSS in core/themes/stable/css/core/dialog

Same with #112 but the paths to the icons need to be slightly different since they should references icons in core/themes/stable/images/core/icons

Fixing both these in this patch.

Cottser’s picture

Status: Needs review » Needs work

Will need a reroll after #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways and probably #2899724: Fixes for settings tray CSS and JavaScript minor issues but there are a couple things that I noticed that can be touched up in the meantime.

  1. +++ b/core/misc/dialog/off-canvas.form.css
    @@ -0,0 +1,137 @@
    +  cursor: pointer;
    

    We need to not add this back please because #2897320: Settings Tray causes the "pointer" cursor to be used both when hovering over inaccessible links (should use "default" cursor) and when in input[type=text] (should use "text" cursor) :)

  2. +++ b/core/modules/system/system.module
    @@ -1515,3 +1518,13 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
     }
    +
    +
    +/**
    

    Minor: Adds one blank line too many.

Cottser’s picture

Also, this patch could be generated with the diff.renames = copies option to shrink the patch and make it easier to review.

tedbow’s picture

Status: Needs work » Needs review
FileSize
83.25 KB

#116.1 fixed
2 fixed

#117 I added the line to my .gitconfig as described in the linked doc but it did not change the size of the file which I had thought it should.

Cottser’s picture

Status: Needs review » Needs work
FileSize
52.17 KB

I diffed the patches, the changes look good, needs a reroll because I committed #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways, might need another one after #2899724: Fixes for settings tray CSS and JavaScript minor issues so whatever works :)

Attaching an example with diff.renames = copies, doesn't apply to HEAD and marking do not test.

tedbow’s picture

Status: Needs work » Needs review
FileSize
82.45 KB

Here is reroll of #118.

Everything in applied except for changes in off-canvas.*.js files.

I manually moved those files and then added the changes from #118 to off-canvas.es6.js and then rebuilt off-canvas.js

Sorry I am still not getting the diff.renames = copies documentation to work for me.

I have

[diff]
  renames = copies

In my ~/.gitconfig file

I run
git config -l
and I see "diff.renames=copies"

I am running git diff like
git diff -M25%

I don't want to derail this issue with git help but if anyone sees what I am doing wrong feel free to ping me on irc or drupal slack.

Status: Needs review » Needs work

The last submitted patch, 120: 2784443-120.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Postponed

#120 does not apply because #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways was reverted.

Not sure it makes sense to reroll until unless that issue is going to take long. I just set it back to RTBC

tedbow’s picture

Status: Postponed » Needs review

Setting back to needs review. Patch should apply now that https://www.drupal.org/node/2830882#comment-12221025 was fixed

Status: Needs review » Needs work

The last submitted patch, 120: 2784443-120.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
82.24 KB

I was wrong patch didn't apply here reroll

Cottser’s picture

Status: Needs review » Needs work

#2899724: Fixes for settings tray CSS and JavaScript minor issues is in now, so hopefully just one more reroll (verified #125 doesn't apply). Thanks @tedbow!

tedbow’s picture

Status: Needs work » Needs review
FileSize
51.72 KB

@Cottser here is the reroll @tim.plunkett finally help me the get the 2784443 thing working also

Status: Needs review » Needs work

The last submitted patch, 127: 2784443-127.patch, failed testing. View results

Cottser’s picture

I'm ignoring the revert on #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways for this review :)

  1. On a general note, the off-canvas.es6.js file refactored in #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways needs some docs cleanup to meet core gates.

        /**
         * The minimum width to use body displace needs to match the width at which
         * the tray will be 100% width. @see core/misc/dialog/off-canvas.css
         * @type {Number}
         */
    

    Blank line should be added above @type line. Not going to call out every instance in the file.

        /**
         * Determines if an element is an off-canvas dialog.
         *
         * @param {jQuery} $element
         *   The dialog element.
         * @return {bool}
         *   True this is currently an off-canvas dialog.
         */
    

    Blank line should be added between @param and @return. Not going to call out every instance in the file.

        /**
         * Handler fired before an off-canvas dialog has been opened.
         * @param  {Object} settings
         *   Settings related to the composition of the dialog.
         * @return {undefined}
         */
        beforeCreate({ settings }) {
    

    There are two spaces after the @param so that it lines up, but we don't do this.

    Also, the @param docs don't seem to line up now that all parameters are being passed in as an object. I may be wrong on this. This may apply to:

    • beforeCreate
    • afterCreate
    • render
  2. +++ b/core/misc/dialog/off-canvas.es6.js
    @@ -2,10 +2,6 @@
    - * @todo This functionality should extracted into a new core library or a part
    - *  of the current drupal.dialog.ajax library.
    - *  https://www.drupal.org/node/2784443
    - *
      * @private
    

    Should the @private be removed too since we're now saying others can use Drupal.offCanvas?

  3. +++ b/core/misc/dialog/off-canvas.form.css
    @@ -74,6 +74,7 @@
    +  cursor: pointer;
    
    +++ b/core/themes/stable/css/core/dialog/off-canvas.form.css
    @@ -74,6 +74,7 @@
    +  cursor: pointer;
    

    This line needs to not come back.

  4. +++ b/core/modules/system/system.module
    @@ -1515,3 +1518,12 @@ function system_query_entity_reference_alter(AlterableInterface $query) {
    +function system_element_info_alter(&$type) {
    +  if (isset($type['page'])) {
    +    $type['page']['#theme_wrappers']['off_canvas_page_wrapper'] = ['#weight' => -1000];
    +  }
    +}
    

    Does this need to be on every page or can we be more selective? This is potentially a breaking change for sites since it would be a markup change for all sites without outside_in enabled (additional div tucked inside <body>).

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
72.49 KB

This patch is against 8.4.x but without the revert of #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways so it won't apply until that is reverted back.
1. fixed
2. fixed
except

Also, the @param docs don't seem to line up now that all parameters are being passed in as an object. I may be wrong on this. This may apply to:

I don't see this. Maybe someone else will.

3. Yes removed private
4. removed
5. Yes it needs to be on every page because off-canvas would be part of the regular dialog api it could be used on any page. Even if it wasn't there when the page loaded new content via ajax could have a off-canvas dialog link.

tedbow’s picture

#2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways committed reuploading patch.

UPDATE:
interdiff-127-130.txt still applies here.
I just applied #127 and then that interdiff

drpal’s picture

From #129.3,

/**
 * beforeCreate
 *
 * @param {Object} obj
 *   This doesn't really matter since it's a fake object without a name anyways.
 * @param {Object} obj.settings
 *   whatever this settings object is for
 */
beforeCreate({ settings }) {
 //...
}

I think if we want to document the destructing of the pseudo-object we can do something like that. Overall the JavaScript looks good; this mostly is just moving things around. 👍

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

@drpal thanks, yeah I'm really not sure how we document things like that. I'd say we can leave it as is and figure it out later :)

  1. +++ b/core/themes/stable/css/core/dialog/off-canvas.form.css
    @@ -74,6 +74,7 @@
    +  cursor: pointer;
    

    This line needs to be removed from stable CSS especially. (#129.3)

  2. +++ b/core/misc/dialog/off-canvas.form.css
    @@ -131,6 +131,5 @@
     #drupal-off-canvas .ui-autocomplete li a {
       color: #595959 !important;
    -  cursor: pointer;
    

    Just curious, why is this line being removed?

  3. The system_element_info_alter() part should probably be mentioned in the change record, not sure it's going to catch people's attention but better than nothing. I'm guessing the main canvas wrapper is inserted server side because inserting it with JS is problematic in some way or less performant or similar?

Also, tagging for manual testing.

xjm’s picture

I see that @tedbow moved this patch back to 8.4.x above, but that should not have happened. After the alpha, committers evaluate issues for backport after commit. We don't ever guarantee that a certain issue will go into a certain branch.

Moving this one to 8.5.x now (again). Thanks @Cottser for the review!

drpal’s picture

@Cottser Yeah. There is surprisingly little information about JSDoc and destructuring objects in functions. I'll perhaps open a followup just to have a place to figure this out for the future since we've already used this pattern in a few other places.

tedbow’s picture

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

#134 sorry moving to 8.4.x I don't think it was intentional(unless I gave a reason can't find comment)

tedbow’s picture

Status: Needs work » Needs review
FileSize
958 bytes
52.53 KB

#133.1 Remvoed. Now diff core/themes/stable/css/core/dialog/off-canvas.form.css core/misc/dialog/off-canvas.form.css is empty
2. This is a mistake and out of scope.

^^ I double checked patch am about upload and "cursor" is not in it at all.

3. will look at change record

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

The only addition is $this->getSession()->wait(800); in the test, the rest are file moves, @internal removals, or s/tray/off-canvas dialog/

I also manually tested the patch by using this with the Layout Builder (after uninstalling Outside In) and it works great.

tedbow’s picture

@tim.plunkett thanks for review 🎉.

#133.3 I updated the change record https://www.drupal.org/node/2896596/revisions/view/10583796/10602357

Cottser’s picture

Thank you @tedbow, the changes look good. And thanks @tim.plunkett for reviewing and testing.

From #133.3:

I'm guessing the main canvas wrapper is inserted server side because inserting it with JS is problematic in some way or less performant or similar?

Could someone please respond to this question? I'm not that worried that the markup change will break people's sites but I have a feeling the folks who like their markup clean probably won't love it.

droplet’s picture

Issue tags: +CSS
FileSize
831.24 KB

@Cottser raised a good question. I always thought `[data-off-canvas-main-canvas]` inserted by outside_in and to avoid particular problems. (e.g. @see setEditModeState in outside_in.es6.js)

There're some new problems I've seen:
1. When it moves to CORE, shouldn't we apply padding to BODY by default? (In fact, shouldn't it apply margin?)

2. Could we call it off-canvas? It changed the main content width. Usually, the off-canvas is a overlay or shift the main content to left/right (with [negative] margin or transform).

3. BODY tag height =/= [data-off-canvas-main-canvas] height. the new wrapper might cause many unexpected issues

4. Shouldn't we add a new class `.off-canvas-opened` to BODY?

5. It could be a headache to a site has a complicated background image across the site elements or use CSS position for elements.

for exmaple, here's one of my client site. It coded with a very common HTML/CSS pattern:

(RED BOX = dialog)

tedbow’s picture

Issue tags: -CSS

@Cottser thanks for our continued work and vigilance on this!

#133.3 I guess don't see the advantage to inserting it via JS

but I have a feeling the folks who like their markup clean probably won't love it.

"markup" here is not just what the server-side produces right? I mean if we insert it via JS it is still markup on the page. Would anybody who doesn't like the idea of the extra div like it any better if we inserted via JS.

Also think it would be much more disruptive if we add via JS. I think the only advantage to adding via JS would be that we could add it conditionally but then that would have some major problems:

  • We would have to determine on any page load whether there were any off_canvas links
  • We would have to check have ajax response to see if it returned new content with off_canvas links

Because of the above reasons the extra div wrapper would much less discoverable. Hopefully contrib and modules in core will start to use the off-canvas dialog. But likely modules would only need the off-canvas dialogs in certain situations so if we only add the div when there is any off-canvas link is on the page then there is the chance site developers won't encounter if the situation in their testing where the off-canvas wrapper is inserted via JS.

Considering a lot of people are not going to read release notes I think this would make it more likely custom theme developers would miss the chance to make sure the extra div is not disruptive if it was added conditionally.

I adding the div wrapper conditionally or on every page via JS could also cause a slight flicker when it is added if the div wrapper makes any visual change at all to the page. Similar to problems that have happened with the toolbar. Even if this didn't happen in core it would be 1 extra thing themes have to deal with.

tedbow’s picture

Issue tags: +CSS

I add an old tab open didn't have @droplet's comment there so I didn't remove the CSS tag intentionally

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 137: 2784443-137.patch, failed testing. View results

drpal’s picture

- #141.4 - In beforeCreate() we already add a class to the body, js-tray-open.

- #141.1,2,3,5 - I think these should be up to the theme to solve with the provided body class.

tedbow’s picture

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

Here is just reroll

re #141
I agree #145 I think js-tray-open will allow theme to do what @droplet is asking for in #141 but I think it would be too much for core to try to solve this problem because we could then look at trying to image other things that themes would do and solve for different situations. I would rather do less and not create CSS that might mess up other situations.