Updated: Comment #0

Problem/Motivation

Several items required when declaring a dialog action for an ajax button could be implied.

Proposed resolution

Code before

$form['add'] = array(
      '#type' => 'submit',
      '#value' => t('Add condition'),
      '#ajax' => array(
        'accepts' => 'application/vnd.drupal-modal',
        'dialog' => array('width' => '750px'),
        'path' => '/admin/structure/pages/edit/' . $entity->id() . '/' . $step . '/condition/add',
        'wrapper' => 'configured-conditions',
        'method' => 'replace',
        'effect' => 'fade',
      ),
    );

Code after

$form['add'] = array(
      '#type' => 'submit',
      '#value' => t('Add condition'),
      '#ajax' => array(
        // Dialog implies an accepts header, we should add it automatically.
        // Use 'modal' => TRUE to indicate that the accept-header should indicate modal (over dialog) 
        'dialog' => array('width' => '750px', 'modal' => TRUE),
        'path' => '/admin/structure/pages/edit/' . $entity->id() . '/' . $step . '/condition/add',
        'wrapper' => 'configured-conditions',
        'method' => 'replace',
        'effect' => 'fade',
      ),
    );

Remaining tasks

Patch

User interface changes

None

API changes

Less verbose

Files: 
CommentFileSizeAuthor
#4 dialog-verbosity-2170541.1.patch4.07 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,410 pass(es). View

Comments

EclipseGc’s picture

Big ++ to this.

sun’s picture

IIRC, a syntax like this was part of the very very original issue/patch that introduced the dialog/modal support in the first place.

I can't remember why it was removed (hopefully not due to my reviews... I vaguely remember some concerns), but in any case, it would certainly be a good idea to look up that prior art. :-)

tim.plunkett’s picture

+1!

Can we also expand this to links?
From #2253257: Use a modal for entity delete operation links:

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -122,6 +122,13 @@ protected function getDefaultOperations(EntityInterface $entity) {
+          'class' => array('use-ajax'),
+          'data-accepts' => 'application/vnd.drupal-modal',
+          'data-dialog-options' => Json::encode(array(
+            'width' => 'auto',
+          )),
larowlan’s picture

Status: Active » Needs review
FileSize
4.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,410 pass(es). View

something like this?

Xano’s picture

+ if (!empty($element['#ajax']['dialog'])) {
That looks like you can't just enable a dialog without any other settings, as dialog would be an empty array.

I know that internally modals are a style of dialog, but I wonder if our API's should not be more generic and treat both the same way, as we may be locking ourselves in a bit too much here. This is more of a thought than something I think must be changed in the patch, though.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Issue tags: +Needs reroll
+++ b/core/includes/ajax.inc
@@ -305,6 +305,19 @@ function ajax_pre_render_element($element) {
+          $accept = 'application/vnd.drupal-dialog';

This change would make the ajax accepts parameter no longer configurable. Are there possibly use cases out there where a different application or accepts header would be used?

Also, I can't tell if this is still an issue as the ajax system has been refactored quite a bit since the last time this was updated.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

Here the results of trying to apply the patch on 8.2.x:

 $ git apply --index dialog-verbosity-2170541.1.patch -v
Checking patch core/includes/ajax.inc...
error: core/includes/ajax.inc: does not exist in index
Checking patch core/includes/common.inc...
error: while searching for:
    $element['#options']['attributes'] += $element['#attributes'];
  }

  // This #pre_render callback can be invoked from inside or outside of a Form
  // API context, and depending on that, a HTML ID may be already set in
  // different locations. #options should have precedence over Form API's #id.

error: patch failed: core/includes/common.inc:3037
error: core/includes/common.inc: patch does not apply
Checking patch core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestController.php...
error: core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestController.php: does not exist in index

I haven't rerolled this patch because I agree with @jhedstrom, this isn't just a simple reroll, instead we should indeed verify we still need to do this.

My findings trying to reroll:
Changes to the AjaxTestController.php which is now at a different location in the file structure no longer seem to make sense to me.
This file now lives on core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php

Changes to common.inc:
drupal_pre_render_link() is now just a wrapper around Link::preRenderLink(), so we could move that part of the patch there.

Changes to ajax.inc:
This file does not exist any more. It looks like ajax_pre_render_element() is now available as Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm so perhaps we could move this change to that method.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

geekinpink’s picture

Assigned: Unassigned » geekinpink
Status: Needs review » Needs work

I have applied the above patch and got same errors as mention in #9 .

geekinpink’s picture

Assigned: geekinpink » Unassigned
droplet’s picture

Issue tags: +Needs JS testing