Skip to content

Conversation

@wuuti
Copy link
Contributor

@wuuti wuuti commented Jul 7, 2017

What does it do?

Adds two lines to the delete confirmation dialogue which show the context the resource is in and the path to and the name of the resource. If the permission to view the ids in the tree is set, also shows the id of the resource in question.

Why is it needed?

The confirmation alert does not show the resource to delete, therefore it may happen that you don't see any more if you clicked on the right resource to delete.

Related issue(s)/PR(s)

The issue was partly described and mentioned in #764 and #13448.

wuuti added 2 commits July 7, 2017 12:31
…resource title.

Implemented new processor which returns the path to a resource.
Added more data centric output to getnodes processor (makes the id-split in js obsolete).
@lawrenz1337
Copy link
Contributor

lawrenz1337 commented Jul 7, 2017

@wuuti I've done a much more simple solution to this in #13497 could you update your repository cause right now it will definitely cause conflicts. I also referenced it is #13448 . Sorry about that :/

@gpsietzema
Copy link
Contributor

#modxbughunt #3points to @wuuti

#modxbughunt #1point to @lawrenz1337

@wuuti
Copy link
Contributor Author

wuuti commented Jul 7, 2017

To note: the problem was solved in parallel by @lawrenz1337 (targeted to 2.x), whereas I targeted 2.5.x. My solution is more complex (introduces a new processor which might be used elsewhere), but gives also more information in the confirmation alert (the path to the resource, the context and the id). If this is considered not necessary by the reviewers, we could drop it. Imho it is useful to see the context and the path and the id, because you may have multiple resources with the same pagetitle (e.g. home in different contexts) - making only displaying the pagetitle not distinctive enough...

@lawrenz1337
Copy link
Contributor

@wuuti My fix show title in a format Delete Pagetitle (ID) so the duplicate pagetitle is not an issue. I haven't noticed that your fix targeted 2.5.x my bad. It will be fine then but this situation in kinda awkward; I like your solution though

@theboxer
Copy link
Member

theboxer commented Jul 7, 2017

I'm closing this in favour of already merged #13497 which solves the issue.

@theboxer theboxer closed this Jul 7, 2017
@theboxer theboxer self-assigned this Jul 7, 2017
@wuuti wuuti deleted the enhance-delete-confirmation branch February 8, 2019 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants