Problem/Motivation

  1. Turn on off_canvas_test module
  2. goto /off-canvas-test-links
  3. click "Click Me 2!"
  4. Try opens with width 555
  5. Without closing tray click "Click me 1!"
  6. Tray stays at width 555 instead of using default

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#28 2828912-28.patch3.49 KBrajeevk
#23 2828912-23.patch3.48 KBrajeevk
#21 2828912-21.patch3.56 KBrajeevk
#20 2828912-20.patch2.4 KBrajeevk
#16 2828912-16.patch3.45 KBtedbow
#16 interdiff-14-16.txt801 bytestedbow
#14 2828912-14.patch3.45 KBtedbow
#14 interdiff-10-14.txt2.4 KBtedbow
#10 interdiff-5-10.txt1.13 KBtedbow
#10 2828912-10.patch2.96 KBtedbow
#10 2828912-10-TEST_ONLY.patch550 bytestedbow
#5 2828912-4-TEST_ONLY.patch947 bytestedbow
#5 2828912-4.patch1.83 KBtedbow
#2 2828912-2-TEST_ONLY.patch902 bytestedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2828912-2-TEST_ONLY.patch902 bytes

Here is failing test that proves the problem.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2828912-2-TEST_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
2828912-4.patch1.83 KB
2828912-4-TEST_ONLY.patch947 bytes

Ok here is fix.

The only way could figure out to do it is explicitly set the width if none is provided. I did this on the php side but could have done it on Javascript.

The problem I see is that now this would always override css but I guess that is fine.

I think the problem is that if you open the tray and then click another link that will be in the tray it is not recreated.

I tried both dialog:beforeclose and dialog:aftercreate js dialog events but no luck.

Status: Needs review » Needs work

The last submitted patch, 5: 2828912-4-TEST_ONLY.patch, failed testing.

The last submitted patch, 5: 2828912-4.patch, failed testing.

The last submitted patch, 5: 2828912-4-TEST_ONLY.patch, failed testing.

The last submitted patch, 5: 2828912-4-TEST_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
2828912-10-TEST_ONLY.patch550 bytes
2828912-10.patch2.96 KB
interdiff-5-10.txt1.13 KB

Forgot to update 2 tests with the new Width option that will come back.

The last submitted patch, 10: 2828912-10-TEST_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Parent issue: » #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it

marking as child of #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it because it relates to the offcanvas tray.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
  1. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
@@ -11,6 +11,8 @@
+  const DIALOG_WIDTH = 300;

    Needs a comment 

  2. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
@@ -42,6 +44,9 @@ public function __construct($title, $content, array $dialog_options = [], $setti
+    if (empty($this->dialogOptions['width'])) {

    Is it ever reasonable to set this to 0? If so, switch to !isset. And either way, probably worth a comment here. 

  3. +++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
@@ -42,6 +44,9 @@ public function __construct($title, $content, array $dialog_options = [], $setti
+      $this->dialogOptions['width'] = self::DIALOG_WIDTH;

    static:: 

  4. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
@@ -59,6 +59,10 @@ public function testOffCanvasLinks() {
           self::assertTrue(strstr($style, 'width: 555px;') !== FALSE, 'Dialog width respected.');
...
+          self::assertTrue(strstr($style, 'width: 555px;') === FALSE, 'Dialog width reset to default.');

    Idk who used self::assertTrue but that should have been $this->assertTrue() to be the same as core, or at the least static::assertTrue()...

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-10-14.txt2.4 KB
2828912-14.patch3.45 KB

@tim.plunkett thanks review. Fixed all 4 points

Idk who used self::assertTrue

I guess this is just 1 of those things that is unknowable, Alas no need to check the previous patch, we will never know.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Er, I meant !isset. Hopefully that patch fails, or we need more coverage.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-14-16.txt801 bytes
2828912-16.patch3.45 KB

@tim.plunkett yep, I should have run the test locally.

Fixed

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.

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs reroll because of #2862625: Rename offcanvas to two words in code and comments. and other recent commits

rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at EX2 Solutions India Pvt Ltd commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2828912-20.patch2.4 KB

Re-rolling patch.

Followed reroll documentation & resolved conflict in file core/module/outside_in/test/src/FunctionalJavascript/OffCanvasTest.php at Step 9.

rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at EX2 Solutions India Pvt Ltd commented
FileSize
2828912-21.patch3.56 KB

Wrong patch...sorry. Missed something in previous one.

Attaching correct one & queuing for test.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Needs work

@RajeevK thanks for the reroll!!

Everything looks good expect this 1 minor thing.

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OffCanvasTest.php
@@ -58,7 +58,12 @@ public function testOffCanvasLinks() {
+          $style = $page->find('css', '.ui-dialog-offcanvas')->getAttribute('style');
...
+          $style = $page->find('css', '.ui-dialog-offcanvas')->getAttribute('style');

.ui-dialog-offcanvas
should be changed to .ui-dialog-off-canvas

This because of class name change in #2862625: Rename offcanvas to two words in code and comments. which was committed since the patch I did in #16

rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at EX2 Solutions India Pvt Ltd commented
Status: Needs work » Needs review
FileSize
2828912-23.patch3.48 KB

@tedbow - Changes done as per your comment. Attaching patch for review & queuing it for test..

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@RajeevK thanks, reroll looks good!

Leaving as needs review for changes introduced in #16(rerolled in #23)

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Issue summary: View changes
a-fro’s picture

a-fro CreditAttribution: a-fro commented

@tedbow I confirmed that the patch applies and works as expected. However, I'd recommend changing the constant name from DIALOG_WIDTH to DEFAULT_DIALOG_WIDTH to make the intention clearer.

rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at EX2 Solutions India Pvt Ltd commented

@tedbow - Let me know if another patch is needed here as per suggestion in #26.

rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at EX2 Solutions India Pvt Ltd commented
FileSize
2828912-28.patch3.49 KB

Attaching another patch with changed constant name to DEFAULT_DIALOG_WIDTH from DIALOG_WIDTH after confirmation from @tedbow.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@a-fro thanks the suggestion I think this a good idea.

@RajeevK thanks for the patch the changes look good.

I think this is ready for RTBC but since I wrote the original patch probably someone else should do it if they agree.

a-fro’s picture

a-fro CreditAttribution: a-fro commented
Status: Needs review » Reviewed & tested by the community

Forgot to mention in #26 that I also confirmed that all of the issues from the code review in #13 have been addressed.

The last submitted patch, 2: 2828912-2-TEST_ONLY.patch, failed testing.

  • lauriii committed 359663b on 8.4.x 
    Issue #2828912 by tedbow, RajeevK, tim.plunkett, a-fro: Offcanvas width...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Version: 8.4.x-dev » 8.3.x-dev

The bug fix looks goods. Committed 359663b and pushed to 8.4.x. Thanks!

Since this only affects an experimental module, I think this could be backported to 8.3.x.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Yep, as an experimental module change, this is backportable to 8.3.x. It changes only code within the module.

  • lauriii committed 3cd15f0 on 8.3.x 
    Issue #2828912 by tedbow, RajeevK, tim.plunkett, a-fro: Offcanvas width...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Reviewed & tested by the community » Fixed

Cherry picked 3cd15f0 and pushed to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: outside_in.module » settings_tray.module

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