Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable confirm modal for catalog > files listing for bulk delete action #16820

Conversation

@matks
Copy link
Contributor

matks commented Dec 15, 2019

Questions Answers
Branch? develop
Description? Introduce new type of Delete Bulk Action for grids, which uses a modal to confirm the user intent, and enable it on catalog > files listing
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes 1 item of #14462 and 1 item of #10550
How to test? Please check that "bulk delete action" on Catalog > files listing uses a modal. No need to rebuild assets. Page can be accessed through BO URL /admin-dev/index.php/sell/attachments/

This change is Reviewable

@matks matks requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 15, 2019
@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Dec 15, 2019

It looks like this:
Capture d’écran 2019-12-15 à 21 20 07

I used a red button instead of the blue from the initial mockup as it was the default bootstrap display and it makes sense for a "delete" action.

@matks matks added the migration label Dec 15, 2019
@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Dec 15, 2019

@marionf You will probably want to check it

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 16, 2019

Thank you @matks !
I just ping @TristanLDD to be sure for the color of the delete button

*/
_onBulkDeleteClick(grid) {
const $deleteBtn = $('.js-common_bulk_delete-grid-action');
const $bulkDeleteForm = $('#' + grid.getId() + '_filter_form');

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 16, 2019

Contributor
Suggested change
const $bulkDeleteForm = $('#' + grid.getId() + '_filter_form');
const $bulkDeleteForm = $(`#${grid.getId()}_filter_form`);
$bulkDeleteForm.attr('action', $deleteBtn.data('form-url'));
$bulkDeleteForm.attr('method', $deleteBtn.data('form-method'));

const $modal = $('#' + grid.getId() + '_grid_confirm_deletion_modal');

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 16, 2019

Contributor

same here

*
* @private
*/
_onBulkDeleteClick(grid) {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 16, 2019

Contributor
Suggested change
_onBulkDeleteClick(grid) {
onBulkDeleteClick(grid) {
* @param {Grid} grid
*/
extend(grid) {
grid.getContainer().on('click', '.js-common_bulk_delete-grid-action', () => this._onBulkDeleteClick(grid));

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 16, 2019

Contributor
Suggested change
grid.getContainer().on('click', '.js-common_bulk_delete-grid-action', () => this._onBulkDeleteClick(grid));
grid.getContainer().on('click', '.js-common_bulk_delete-grid-action', () => this.onBulkDeleteClick(grid));
@@ -33,6 +33,7 @@ import SubmitBulkExtension from '@components/grid/extension/submit-bulk-action-e
import SubmitRowActionExtension from '@components/grid/extension/action/row/submit-row-action-extension';
import LinkRowActionExtension from '@components/grid/extension/link-row-action-extension';
import TranslatableInput from '@components/translatable-input';
import BulkDeleteWithModalConfirmExtension from '@components/grid/extension/action/bulk/bulk-delete-with-confirm-modal-extension';

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 16, 2019

Contributor
Suggested change
import BulkDeleteWithModalConfirmExtension from '@components/grid/extension/action/bulk/bulk-delete-with-confirm-modal-extension';
import BulkDeleteWithModalConfirmExtension
from '@components/grid/extension/action/bulk/bulk-delete-with-confirm-modal-extension';
'submit_route',
])
->setDefaults([
'submit_method' => 'POST',

This comment has been minimized.

Copy link
@PierreRambaud

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Dec 19, 2019

Contributor

(and for submit_method)

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 16, 2019

I think it's a pity to limit this modification to bulk delete It's almost exactly the same behaviour as a submit button except the modal is prettier I suggest making a more generic component to manage the modal I'll propose a PR based on your branch this will be easier to explain

@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Dec 16, 2019

I think it's a pity to limit this modification to bulk delete It's almost exactly the same behaviour as a submit button except the modal is prettier I suggest making a more generic component to manage the modal I'll propose a PR based on your branch this will be easier to explain

I thought about it but I'm not sure making it generic is a good idea.

There is actually already 2 other "confirm before bulk delete" modals that already exist: categories and customers. They have a few more specific logic but they are definitely "confirm before bulk delete" usecase.

When I started this feature I tried to make something generic, so that we would have a single configurable modal that would fit all usecases. I came close to doing it ... but the code was full of if, options and generic / badly named variable names. The code was hard to build, hard to read and I expected it to become hard to maintain.

That is why I took the exact reverse path and went for something 100% specific, following KISS principle: keep it simple and stupid.

If you want to make it a configurable modal, you'll probably need to pass, as options:

  • selectors to target/listen for jQuery
  • routes to call, with parameters and maybe arguments / parameters mapping
  • wordings, either already translated or along with their translation domain
  • classes to apply for different kind of buttons (in my case I had to apply text-left font-weight-normal CSS classes to modal body to make it look correct)
  • accept different kind of content for the modal body, including html (such as <p> parts), and handle different sizes

... 😅 see what I mean ?

And if you define all these options from the GridDefinition (because I see no else location), it's one more step towards mixing backend logic and view configuration, violating the MVC principle (we already violate it for several items, but this would be one more)

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 17, 2019

@matks Mmm, maybe I see it too simple in my mind and don't see the big picture But I thought of something really simple:

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 17, 2019

@matks Mmm, maybe I see it too simple in my mind and don't see the big picture But I thought of something really simple:

  • a generic modal template that would be present in gridActions template so available for every use case (some are already present)
  • modify the SubmitBulkActionExtension js so that instead of using the native confirm it uses a modal
  • the question is already in the options and would be inserted inside the modal content (just one text)
  • we would only need to add an option for title to insert in the modal, it would be optional and we use the question/content as a default title if not present

The url and action already already managed anyway so nothing to add!

Then it wouldn't be that complicated, better it would make the modification for ALL submit actions at once, the only thing to do would be to add the title in each grid definitions (optional but prettier)

What do you think? As I suggested I can try a proposal on a branch based on your so you can see the difference and if it's too complicated

$bulkDeleteForm.attr('action', $deleteBtn.data('form-url'));
$bulkDeleteForm.attr('method', $deleteBtn.data('form-method'));
Comment on lines 54 to 55

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Dec 19, 2019

Contributor
$bulkDeleteForm.attr({
  action: $deleteBtn.data('form-url'),
  method: $deleteBtn.data('method')
});
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 19, 2019

@marionf I have a question for you, we integrated the possibility to use a modal instead of native confirm It can be activated on any bulk action now, with some options:

  • modal title
  • confirm button label
  • confirm button class

Here is how it looks like for a customized one on delete:

Capture d’écran 2019-12-19 à 13 19 53

And the default one would like this:

Capture d’écran 2019-12-19 à 10 52 04

So the question is, do we enable the modal by default on every bulk action? This would enable it on all migrated pages. Or do we keep the native confirm?

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 19, 2019

The idea is to have the same modal to confirm or cancel deletion on all migrated pages
The modal can be used only for delete action (in bulk or for only 1 item)

$confirmButton.className = 'btn btn-confirm-submit btn-lg';
$confirmButton.addClass(confirmButtonClass);
$confirmButton.html(confirmButtonLabel);
$confirmButton.off('click').on('click', () => this.postForm($submitBtn, grid));

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 19, 2019

Contributor

Still thiking .off('click') isn't a good idea and can break all modules javascript event on the confirm button..

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 19, 2019

Contributor

It is required because the same modal can be shared between several submit actions, so we need to remove the previous callback to be sure only one will be called on confirm
If module need to use the same modal then they will have to do the same cleaning

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 19, 2019

Contributor

A new modal must be created depending on the action you do. You shouldn't share the same modal context, more if you allow people to add module on your app 😥
If a module want to add a custom action before the this.postForm and just after the first 'click' event, he must reimplement everything. Sad story :(

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 19, 2019

Contributor

Mmm I think you are mixing too many features here for a use case that is much more simple in this case

This modal is simply a "pretty confirm" You can change the question, title, button style and that's it It's not a modal that any other module is supposed to listen to Just like when you use confirm in your code no modules will be waiting for the return

We don't want to module to integrate in this modal, since we have absolutely no JS hooks, event, or module integration available today anyway

However what this PR will allow is for module to add their SubmitBulkAction in the grid definition and be able to use a pretty modal only using a PHP hook

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 20, 2019

Contributor

Even if we don't have hooks, we are able to add custom javascript files in the BO, extend the theme, or anything else. This means you're able to add a custom .on('click') on the confirm button to trigger a special api request or a custom effect, a new ajax box loaded.
This is also why we are not able to change all our html ids even if we don't have custom html hooks :trollface:

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 20, 2019

Contributor

And indeed as Mathieu said, if one wants to override the default behavior he can always unbind our actions on the submit button

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 20, 2019

Contributor

This is exactly what I said. If he wants to keep the original behaviour and add a little action just before or after the event. He must reimplement what you've done, this means duplicate the postForm method.
@matks It's not targetting the .btn-confirm-submit, it targets the '#GRID_ID_grid_confirm_modal .btn-confirm-submit which is different.

As I said earlier, you disallow the capability to add an event when you click on the button, I keep thinking it's really bad for front developers :/
Two solutions:

  1. Generate the modal dynamically, so no one can add custom event on it
  2. One modal per action

Don't forget people can add javascript, and there is no hook / event in the BO for front developers, don't hand them the stick :(

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 20, 2019

Contributor

I agree with Pierre, let peoples be ables to add events on buttons, don't kick events off, it won't change anything about your behavior no ?

Peoples may wants to add a behavior to this button and not delete your one, if they wants to do this, they need to remove your event, add your own behavior again and then add their behavior, it doesn't look like it's a good practice at all

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 20, 2019

Contributor

But I don't remove anything!! Before you add just a confirm call, and absolutely NO way to integrate a behaviour before or after this confirm You are mixing things here, I didn't remove the possibility to add any behaviour on the bulk button
This confirm button is a new button in a new modal that no one has ever seen or has ever modified

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 20, 2019

Contributor

This modal can be seen as a blackbox modal confirm component, and has no intention of allowing internal customization so indeed I don't want external modules to change its behaviour

@jolelievre jolelievre force-pushed the matks:add-modal-confirm-deletion-to-catalog-files-listing branch from cd3c54a to d5fc79b Dec 19, 2019
NeOMakinG added 3 commits Dec 30, 2019
Copy link
Contributor

jolelievre left a comment

Nice work @NeOMakinG This will be useful in some other places I'm sure Just a few feedback to make it even more awesome ;)

NeOMakinG added 4 commits Dec 30, 2019
@boubkerbribri

This comment has been minimized.

Copy link
Contributor

boubkerbribri commented Jan 8, 2020

I just added a pull request #17066 to update test when this PR is merged. please make sure to validate it before merging this one (otherwise, functional test will fail)

@Progi1984 Progi1984 added this to the 1.7.7.0 milestone Jan 8, 2020
* International Registered Trademark & Property of PrestaShop SA
*/

const $ = window.$;

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor
Suggested change
const $ = window.$;
const {$} = window;
@TristanLDD

This comment has been minimized.

Copy link

TristanLDD commented Jan 8, 2020

I agree for the color of the delete button, it's more safe to have it in red.

@TristanLDD TristanLDD added UX ✔️ and removed waiting for UX labels Jan 8, 2020
* @param {Function} confirmCallback
*
*/
function Modal({id = 'confirm_modal', confirmTitle, confirmMessage = '', closeButtonLabel = 'Close', confirmButtonLabel = 'Accept', confirmButtonClass = 'btn-primary'}) {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor
Suggested change
function Modal({id = 'confirm_modal', confirmTitle, confirmMessage = '', closeButtonLabel = 'Close', confirmButtonLabel = 'Accept', confirmButtonClass = 'btn-primary'}) {
function Modal(
{
id = 'confirm_modal',
confirmTitle, confirmMessage = '',
closeButtonLabel = 'Close',
confirmButtonLabel = 'Accept',
confirmButtonClass = 'btn-primary',
},
) {
/**
* Modal component to improve lisibility by constructing the modal outside the main function
*
* @param {String} id

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor

There is only one param, it's an object

export default function ConfirmModal(params, confirmCallback) {
// Construct the modal
const {id, closable} = params;
const modal = new Modal(params);

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor
Suggested change
const modal = new Modal(params);
this.modal = Modal(params);

No need new instruction here

*/
function Modal({id = 'confirm_modal', confirmTitle, confirmMessage = '', closeButtonLabel = 'Close', confirmButtonLabel = 'Accept', confirmButtonClass = 'btn-primary'}) {
// Main modal element
this.modal = document.createElement('div');

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor

no need to use this in this function

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor

Return an object in this function. something like

return {modal: {
  dialog: ...,
  confirmation: ....,
}

don't waste time by repeating modal in variables names

this.$modal.modal();
};

this.modalConfirmButton.addEventListener('click', confirmCallback);

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jan 8, 2020

Contributor
Suggested change
this.modalConfirmButton.addEventListener('click', confirmCallback);
this.modal.confirmation.addEventListener('click', confirmCallback);
@jolelievre jolelievre merged commit c60a424 into PrestaShop:develop Jan 8, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.