Allow dialog links to specify a renderer in addition to a dialog type

1. +++ b/core/misc/ajax.es6.js
   @@ -526,7 +527,11 @@
   +    let wrapper = `drupal_${(element_settings.dialogType || 'ajax')}`;
   

   Can we put this conditional value where the property is declared?

2. +++ b/core/modules/outside_in/js/outside_in.es6.js
   @@ -211,28 +211,18 @@
   +          return hasElement && rendererOffCanvas;
   

   This is slightly confusing syntax.

@tedbow

Some minor nits, but it looks like the tests don't pass.

+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -211,28 +211,17 @@
+      // Loop through all Ajax links to set active editable ID.

Should probably be documentation about the filter expression

Only the smallest of nitpicks:

+++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php
@@ -0,0 +1,77 @@
+   * WideModalRenderer constructor.
+   * @param \Drupal\Core\Controller\TitleResolverInterface $title_resolver

"Constructs a new WideModalRenderer." and needs a blank line.

The rest looks great.

1. +++ b/core/misc/ajax.es6.js
   @@ -60,6 +60,7 @@
   diff --git a/core/misc/ajax.es6.js.orig b/core/misc/ajax.es6.js.orig
   

   I don't think we need this file.

2. +++ b/core/misc/ajax.es6.js.orig
   @@ -0,0 +1,1334 @@
   diff --git a/core/misc/ajax.es6.js.rej b/core/misc/ajax.es6.js.rej
   

   Or this file.

1. +++ b/core/misc/ajax.es6.js
   @@ -517,7 +518,13 @@
   +    // If this element has a dialog type use if for the wrapper if not use 'ajax'
   

   Nit: Missing trailing period.

2. +++ b/core/modules/outside_in/js/outside_in.es6.js
   @@ -210,21 +210,17 @@
   +            // If there is an element and the renderer is 'off_canvas' then we want to add our changes.
   ...
   +        // Loop through all Ajax instances that use the 'off_canvas' renderer to set active editable ID.
   

   Nit: 80 cols.

3. +++ b/core/modules/outside_in/outside_in.services.yml
   @@ -3,7 +3,7 @@ services:
        class: Drupal\outside_in\Render\MainContent\OffCanvasRender
   

   This must be named OffCanvasRenderer.

4. +++ b/core/modules/system/tests/modules/dialog_renderer_test/dialog_renderer_test.services.yml
   @@ -0,0 +1,13 @@
   +# Provide 2 render services that use the same class but behave differently
   

   s/render/main content renderer/

5. +++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Controller/TestController.php
   @@ -0,0 +1,80 @@
   +          'data-dialog-type' => 'modal',
   ...
   +          'data-dialog-type' => 'modal',
   +          'data-dialog-renderer' => 'wide',
   ...
   +          'data-dialog-type' => 'modal',
   +          'data-dialog-renderer' => 'extra_wide',
   

   Looks good.

6. +++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php
   @@ -0,0 +1,77 @@
   + * Wide main content renderer for modal dialog requests.
   

   For consistency with the existing ones:

   Default main content renderer for wide modal dialog requests.
   

7. +++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php
   @@ -0,0 +1,77 @@
   + * This test class is copied from \Drupal\Core\Render\MainContent\ModalRenderer
   ...
   +class WideModalRenderer extends DialogRenderer {
   

   Why is this not extending ModalRenderer?

8. +++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php
   @@ -0,0 +1,77 @@
   +   * WideModalRenderer constructor.
   +   * @param \Drupal\Core\Controller\TitleResolverInterface $title_resolver
   

   Nit: Needs blank line in between.

9. +++ b/core/modules/system/tests/src/FunctionalJavascript/ModalRendererTest.php
   @@ -0,0 +1,38 @@
   +    $style = $session_assert->waitForElementVisible('css', '.ui-dialog')->getAttribute('style');
   +    $this->assertNotContains('700px', $style);
   +    $this->assertNotContains('1000px', $style);
   +    $this->drupalGet('/dialog_renderer-test-links');
   +    $this->clickLink('Wide Modal!');
   +    $this->assertNotEmpty($session_assert->waitForElementVisible('css', '.ui-dialog[style*="width: 700px;"]'));
   +    $this->drupalGet('/dialog_renderer-test-links');
   +    $this->clickLink('Extra Wide Modal!');
   +    $this->assertNotEmpty($session_assert->waitForElementVisible('css', '.ui-dialog[style*="width: 1000px;"]'));
   

   Interesting way of testing this, seems sound.

- #26.1,2,3,4,5,6,8,9 👍
- #26.7 🤷‍♀️
- Fixed an issue with two missing variables that were accidently removed in #21/#23, search & replace
- Simplified Drupal.ajax.instances filter.

+++ b/core/modules/system/tests/modules/dialog_renderer_test/src/Render/MainContent/WideModalRenderer.php
@@ -0,0 +1,78 @@
+   * WideModalRenderer constructor.
+   * ¶

This is still wrong, needs to be something like "Constructs a new WideModalRenderer." and also there is now a trailing whitespace on the blank line

One nit, one question.

1. +++ b/core/modules/outside_in/js/outside_in.es6.js
   @@ -199,7 +199,8 @@
   -   * Toggle the js-outside-edit-mode class on items that we want to disable while in edit mode.
   +   * Toggle the js-outside-edit-mode class on items that we want to disable
   +   * while in edit mode.
   

   Nit: Must fit on single line.

2. +++ b/core/modules/outside_in/outside_in.services.yml
   @@ -1,9 +1,9 @@
      main_content_renderer.off_canvas:
   
   +++ b/core/modules/outside_in/src/Render/MainContent/OffCanvasRenderer.php
   @@ -13,7 +13,7 @@
    /**
     * Default main content renderer for off-canvas dialog requests.
     */
   

   Why is this still called "off canvas renderer"? Isn't that a remnant from when this module was called off_canvas.module?

   Is the plan to move this into core itself, outside this module, once this module becomes stable?

@Wim Leers

#33.1 - Really?

"Nit".

It's a coding standard rule we have. Maybe that changed with the shift to ES6? Not clear to me. Anyway, just a nit — should not hold up commit.

It's a coding standard rule we have. Maybe that changed with the shift to ES6? Not clear to me. Anyway, just a nit — should not hold up commit.

No changes to ES6 as I known. It's same as PHP I think..

/** this style allow multiple line **/
// this one should be a single line no matter how loooooooooooooooooooooooooooooooooooog it is.

#36 Ok.
#38 Yay, that ends that yak shaving exercise immediately!

This patch looks great. My only question is:

+++ b/core/misc/ajax.es6.js
@@ -60,6 +60,7 @@
+        element_settings.drupalDialogRenderer = $(this).data('dialog-renderer');

Why are we prefixing this with "drupal"? Why not just element_settings.dialogRenderer?

Also, I'm not entirely thrilled with the word "renderer" in the attribute name, because when I think of "what's rendering the dialog?", I think of Drupal.dialog() or jQuery UI's dialog() function, and the value of 'data-dialog-renderer' isn't changing that (it could in theory, depending on the implementation of the PHP service that binds to this, but the off_canvas "renderer" is using the same jQuery UI dialog() function for client-side rendering that the base dialog type does). It's a "renderer" on the PHP side, but HTML attribute names and JS code are in the client-side world, and in that world, I don't see "off_canvas" as a separate renderer. Looks to me like it's more like a dialog "style", but we shouldn't use that word either, since that would lead people to think of CSS, rather than "style" in the more abstract sense.

I can't think of a better word, so won't hold this up on that.

Actually, for #41, what do you all think of "preset"? I.e., "data-dialog-preset" => "off_canvas"? I don't know if that helps to clarify or not, so feel free to shoot it down.

That machine name corresponds to a class that implements \Drupal\Core\Render\MainContent\MainContentRendererInterface, which says

* The interface for "main content" renderers.

Example classes, and their corresponding machine name
drupal_ajax => \Drupal\Core\Render\MainContent\AjaxRenderer
drupal_modal => \Drupal\Core\Render\MainContent\ModalRenderer
drupal_dialog => \Drupal\Core\Render\MainContent\DialogRenderer
drupal_dialog_off_canvas => \Drupal\outside_in\Render\MainContent\OffCanvasRender

That machine name corresponds to a class that implements \Drupal\Core\Render\MainContent\MainContentRendererInterface

Right, per #41, that's PHP-centric. But we're talking about a name for an HTML attribute and JS variable. How that's connected back to PHP is an implementation detail.

Thanks @tedbow!

Thanks for #45, and I see your point in #47, so I'm about to commit this. Ticking credit boxes for reviewers.

Pushed to 8.4.x.

+++ b/core/misc/ajax.es6.js
@@ -517,7 +518,13 @@
-    ajax.options.url += `${Drupal.ajax.WRAPPER_FORMAT}=drupal_${element_settings.dialogType || 'ajax'}`;
+    // If this element has a dialog type use if for the wrapper if not use 'ajax'.
+    let wrapper = `drupal_${(element_settings.dialogType || 'ajax')}`;
+    if (element_settings.dialogRenderer) {
+      wrapper += `.${element_settings.dialogRenderer}`;
+    }
+    ajax.options.url += `${Drupal.ajax.WRAPPER_FORMAT}=${wrapper}`;

There might be a side effect to having committed this. ExceptionJsonSubscriber::getHandledFormats() has a hard-coded list of formats. drupal_dialog.off_canvas isn't one of them, so errors within such a request might not get handled/formatted correctly. This could use a follow-up issue for solving.

#52 Wow, great catch!