Problem/Motivation

OpenOffCanvasDialogCommand::__construct() is documented as saying "Drupal provides a built-in offcanvas tray for this purpose, so no selector needs to be provided.", but it then proceeds to pass one (#drupal-offcanvas) to the parent constructor. Meanwhile, this is an ID that doesn't match any element. And the JS code in Drupal.AjaxCommands.prototype.openOffCanvas doesn't use the returned selector anyway. This is all confusing.

Proposed resolution

  • Pass NULL to the parent constructor instead.
  • Consider also simplifying OpenOffCanvasDialogCommand::render() to invoke the parent method and only change what needs changing.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#4 2785133-4.patch1.46 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

tedbow’s picture

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

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Parent issue: #2762505: Introduce "outside in" quick edit pattern to core » #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
Issue tags: -Outside-in
FileSize
2785133-4.patch1.46 KB

OpenOffCanvasDialogCommand::__construct() is documented as saying "Drupal provides a built-in offcanvas tray for this purpose, so no selector needs to be provided.", but it then proceeds to pass one (#drupal-offcanvas) to the parent constructor.

I have update the comment. Since we are using the dialog system now we need the hard-coded selector as OpenModalDialogCommand does.

And the JS code in Drupal.AjaxCommands.prototype.openOffCanvas doesn't use the returned selector anyway. This is all confusing.

Yes it was probably not using but now the dialog system is. In dialog.ajax.js

  Drupal.AjaxCommands.prototype.openDialog = function (ajax, response, status) {
    if (!response.selector) {
      return false;
    }
Consider also simplifying OpenOffCanvasDialogCommand::render() to invoke the parent method and only change what needs changing.

This patch does that.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This is clearly the correct thing to do --- nice cleanup!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed c225f3b to 8.3.x and 2c81806 to 8.2.x. Thanks!

Settings tray is an experimental module so this goes in 8.2.x too.

  • alexpott committed c225f3b on 8.3.x 
    Issue #2785133 by tedbow, effulgentsia: Simplify...

  • alexpott committed 2c81806 on 8.2.x 
    Issue #2785133 by tedbow, effulgentsia: Simplify...
tedbow’s picture

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

@alexpott 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! :)