Update entity delete operation links to use a modal by default, as delete forms are simple confirm forms in 99.99% of all cases (that 0.01% is just so nobody can tell me I forgot anyone :-P )

Comments

Xano’s picture

Status: Active » Needs review
FileSize
1.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,716 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: drupal_2253257_1.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,853 pass(es). View
larowlan’s picture

Looks like the modal title isn't coming through correctly.
I would expect to see the question 'Are you sure you want to delete' etc.
screenshot

Xano’s picture

That is because of #2255369: DialogController does not respect $content['#title'] for the page title. The question doesn't fit the space the modal reserves for the title, though.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -123,6 +123,10 @@ protected function getDefaultOperations(EntityInterface $entity) {
+          'class' => array('use-ajax'),
+          'data-accepts' => 'application/vnd.drupal-modal',

If you add

      'data-dialog-options' => Json::encode(array(
        'width' => 'auto',
      )),

Then it will fit just fine.

Xano’s picture

Issue summary: View changes
FileSize
819 bytes
2.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_2253257_8.patch. Unable to apply patch. See the log in the details link for more information. View

Many entity delete pages have <em> in their titles, which isn't properly displayed by the modal, but that's for another issue as well.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_2253257_8.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

8: drupal_2253257_8.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_2253257_8.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,323 pass(es), 3 fail(s), and 0 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 12: drupal_2253257_12.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
3.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal_2253257_14.patch. Unable to apply patch. See the log in the details link for more information. View
tim.plunkett’s picture

+++ 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',
+          )),

Out of scope for this issue, but it'd be realllly nice if we had a way to just have 'class' => array('use-modal'), and it would expand to all of this.

larowlan’s picture

Xano’s picture

14: drupal_2253257_14.patch queued for re-testing.

Xano’s picture

Since we have to use somewhat more verbose code until that other issue is fixed, do we want to postpone this one, or just get it in?

14: drupal_2253257_14.patch queued for re-testing.

Xano’s picture

14: drupal_2253257_14.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 14: drupal_2253257_14.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,479 pass(es). View

Re-roll.

wiifm’s picture

Status: Needs review » Needs work
Issue tags: +#Drupal8nz
FileSize
33.16 KB
49.76 KB

Steps to test:

Visit /admin/structure/types, click delete on the content types

Content type with content:

Issues:

  • Needs an action button to dismiss the dialog (not just a close cross in the corner)
  • Double escaped HTML in the dialog header

Content type with no content:

Issues:

  • Cancel link should be next to the delete link - seems weird in the modal body
  • Double escaped HTML in the dialog header
larowlan’s picture

Issue tags: +Needs reroll
swentel’s picture

Issue tags: -Needs reroll
FileSize
3.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,728 pass(es). View

rerolled - not sure what the best strategy is for the escaped titles.

dawehner’s picture

Isn't that the same kind of problem as what got tackled on #2207247: Dialog titles double escaped for views handlers and delete forms ?

swentel’s picture

@dawehner indeed

adci_contributor’s picture

Status: Needs work » Needs review

rerolled

Guess we should sending it to testbot.

dawehner’s picture

Status: Needs review » Postponed

The other one seems to have an okay from nod_, so let's postpone this issue on top of the other one?

mgifford’s picture

Berdir’s picture

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

And.. its one year later ;)

I've just RTBC'd #2207247: Dialog titles double escaped for views handlers and delete forms as nobody else wanted to do it. Maybe we can try this again for 8.x-1.x. Note that many config delete forms got quite a bit bigger due to config dependency information but I guess this still makes sense.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

jhedstrom’s picture

Status: Postponed » Needs review
Issue tags: +Needs tests
FileSize
3.2 KB

This is just a re-roll of #24--it doesn't appear to be working though as something must have changed in the past few years. We can also now add javascript tests for this. I came across this issue while working on #2828201: Add a modal form option to the confirm and field entry form action link types.

Berdir’s picture

See [#2488192], you want data-dialog-type now, no longer the arcane accepts header.

jhedstrom’s picture

Thanks @berdir!

This is now working--still probably needs a js test though.

The last submitted patch, 33: 2253257-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2253257-35.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
4.51 KB

Fixes the failing tests.

jhedstrom’s picture

FileSize
2.43 KB
6.95 KB

This has the start of a test. See the @todo for some odd behavior. I'd expect a new page load with a confirmation message on submit. However, the entity does appear to be deleted...

Status: Needs review » Needs work

The last submitted patch, 39: 2253257-39.patch, failed testing.

jhedstrom’s picture

That's a failure I haven't come across before:

Current page is "/entity_test/list", but "/checkout/entity_test/list" expected.

I would have thought that $this->assertSession()->addressEquals(Url::fromRoute('entity.entity_test.collection')->toString()); would take a subdirectory into account...

Berdir’s picture

@jhedstrom: Looks to me like it fails *because* it takes that into account: We actually are on /checkout/entity_test/list and it sees just /entity_test/list but toString() includes the directory. Try '/' . getInternalPath(), or just hardcode it..

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
6.98 KB

Ah, I was reading that backwards. I made the change suggested in #42, and have also changed to not use the loadUnchanged(), and instead just manually cleared the cache (there is an @todo on the method that suggests its behavior will change in the future).

I am still not sure why the page isn't reloading after clicking on the delete button.

jhedstrom’s picture

Issue tags: -Needs tests

I am still not sure why the page isn't reloading after clicking on the delete button.

I figured out why this appears to be the case. Apparently when using clickLink, the verbose page visit logging does not take place, so it only appeared there was no page reload when I was looking at the pages generated by the html output printer.

jhedstrom’s picture

FileSize
877 bytes
6.93 KB

Given #44, this removes the @todo and just checks for the delete message to be present on the page.

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.

Munavijayalakshmi’s picture

Rerolled the patch #45.

droplet’s picture

Love it.

there's a little problem which is similar to this: #2741877: Nested modals don't work: when using CKEditor in a modal, then clicking the image button opens another modal, which closes the original modal

We can assign a new selector to avoid this problem. @see #2741877 Comment 3. (also @see #2741877 Comment 12 for my points)

Thanks.