Update the entity form delete link to use a modal by default, as delete forms are simple confirm forms in 99.99% of all cased (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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,350 pass(es). View
larowlan’s picture

Looks like the question isn't coming through as the title.
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.

Xano’s picture

FileSize
670 bytes
1.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,296 pass(es). View

This makes sure that long titles are displayed correctly by the modal. However, 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.

larowlan’s picture

Xano’s picture

So is this good to go?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Yes

olli’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-2254935-8.patch. Unable to apply patch. See the log in the details link for more information. View
1.61 KB
+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -62,6 +63,13 @@ public static function buildCancelLink(ConfirmFormInterface $form, Request $requ
+      'data-dialog-options' => Json::encode(array(
+        'width' => 'auto',
+      )),

Shouldn't this be in the delete link?

Xano’s picture

Status: Needs review » Reviewed & tested by the community

You're right. Thanks for being so observant!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: drupal-2254935-8.patch, failed testing.

Xano’s picture

Status: Needs work » Reviewed & tested by the community

Ugh. Another testbot malfunction.

alexpott’s picture

8: drupal-2254935-8.patch queued for re-testing.

Bojhan’s picture

I am not a huge fan of confirm dialogs, as they turn into a habit even when it concerns critical messages. However I don't see the current step as a bad thing. I have a few questions:

  • Does this work well on mobile? I am not sure how well tested our confirm dialogs are.
  • Shouldn't we also do this with configuration forms, it seems like going without that will create quite a inconsistency. Also I assume that will be much harder.
Xano’s picture

I have no idea how configuration forms are related to this issue, as they are not entity forms and have no delete links. Can you elaborate on that?

LewisNyman’s picture

The confirm dialog is a global component, if it's not mobile friendly yet then we need to fix that globally. It shouldn't affect the decision to use a dialog on a case per case basis.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
30.72 KB

Deleting something should follow a consistent pattern, regardless if it's an entity or not. For example, deleting an URL alias has a confirmation step similar to the one for entities:

Screengrab of URL alias edit form

yoroy’s picture

Lewisnyman: the question is whether a case-by-case approach helps us closer to release. Not sure it does.

Xano’s picture

This patch does not remove the confirmation step. It just presents that step as a modal rather than a separate page when people click the Delete link from the entity form (#2253257: Use a modal for config entity delete operation links is for the operation link in entity lists). Is this what you mean? I just want to make sure since there has been some confusion.

If this is what you mean, do you have any suggestions? Should we keep confirmation pages for deletion workflows, or should we implement modals across the board straight away?

yoroy’s picture

Understood that the confirmation step stays. I meant to say that *the process of deleting something* (entity or whatever) in the UI should follow the same pattern. Moving it into a modal for only some of the delete operations makes it less consistent.

I have no objections to the use of the modal itself. Just cautious to apply it only in some places, some not.

yoroy’s picture

Ok, so best next step seems to be to get a list of the non-entity delete forms in core that would not be touched by this patch so that we can see how many exceptions we'd introduce and assess how bad that might be.

Xano’s picture

Agreed.

Xano’s picture

Berdir queued 8: drupal-2254935-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: drupal-2254935-8.patch, failed testing.

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.

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.

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.

RoloDMonkey’s picture

Version: 8.3.x-dev » 8.4.x-dev
Assigned: Unassigned » RoloDMonkey

Since the parent issue is for 8.4, I am updating this one to be the same.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
111.6 KB
1.71 KB

Re-roll against 8.4.x.

vijaycs85’s picture

FileSize
69.79 KB

Also the space between the delete link and loading icon seems bit too much. Attaching screenshot.

Status: Needs review » Needs work

The last submitted patch, 29: 2254935-29.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
964 bytes
2.65 KB

Updating test fail.

droplet’s picture

Issue tags: +Needs JS testing

Jot a quick note:

I think we need to test nested Modal with this new feature.

Status: Needs review » Needs work

The last submitted patch, 32: 2254935-32.patch, failed testing.

Manuel Garcia’s picture

Assigned: RoloDMonkey » Unassigned
Status: Needs work » Needs review
FileSize
1.79 KB
3.74 KB

Fixing the failing tests, and expanding a bit the tests on there to check for the modals.

Re: #33: I think nested modals are not working in core at the moment, see #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

vijaycs85’s picture

vijaycs85’s picture

Title: Use a modal for the entity form delete link » Use a modal for content entity form delete link
yoroy’s picture

One quick note is that this only seems to work from the delete link while already editing the thing. There's a "Delete" tab on the front-end view of content items where I'd expect the same behaviour but no modal there (yet).

Manuel Garcia’s picture

@yoroy my understanding is that this issue is for the entity edit form only.

Another place that is not covered by this patch would be the delete link on the operations links (for example on the /admin/content view)

In any case, I agree from a UX perspective that we should do this for any link pointing to a delete entity form so that it is the same expected behavior.

I guess the question is: Should we expand the scope of this issue or not?

RoloDMonkey’s picture

The other ways to delete content are already being handled in separate issues. Please scroll to the top of this issue and see the parent issue and related issues.

yoroy’s picture

Once more proof people don't see things in sidebars :) I see that is covered indeed.