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

EZP-29747: As an editor, I want to see a list of content items impossible to delete #712

Conversation

@tischsoic
Copy link
Contributor

commented Nov 9, 2018

Question Answer
Tickets https://jira.ez.no/browse/EZP-29747
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Implements modal for bulk delete/move with list of failed items.

Related PR in ezplatform-admin-ui-modules: ezsystems/ezplatform-admin-ui-modules#113

Screenshots

screen shot 2018-12-06 at 17 15 11

screen shot 2018-12-12 at 15 47 57

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@tischsoic tischsoic self-assigned this Nov 9, 2018

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from d7cf816 to c4141a8 Nov 9, 2018

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from c4141a8 to 93a152a Dec 4, 2018

@tischsoic tischsoic changed the title WIP EZP-29747: As an editor, I want to see a list of content items impossible to delete EZP-29747: As an editor, I want to see a list of content items impossible to delete Dec 7, 2018

Show resolved Hide resolved src/bundle/Resources/public/js/scripts/admin.location.view.js Outdated
Show resolved Hide resolved ...e/Resources/views/content/modal_location_bulk_operation_failed.html.twig Outdated
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">

This comment has been minimized.

Copy link
@dew326

dew326 Dec 10, 2018

Member

Not needed. Can be in one line

tabindex="-1"
role="dialog">
<div
class="modal-dialog"

This comment has been minimized.

Copy link
@dew326

dew326 Dec 10, 2018

Member

This is less readable. If the line is really long it's fine to split it in few but in this case, (and in some other here) I think it's not needed.

@tischsoic

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Changes applied in the last commit - ping @dew326 .

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from d72c4c5 to 30526e9 Dec 10, 2018

@micszo micszo self-assigned this Dec 11, 2018

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch 3 times, most recently from 36d2f1b to 258bb67 Dec 11, 2018

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from 258bb67 to d7a156a Dec 11, 2018

@@ -1,4 +1,5 @@
(function(global, doc, $, React, ReactDOM, eZ, Routing) {
const SELECTOR_BULK_OPERATION_FAILED_MODAL = '.ez-bulk-operation-failed-modal';

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

SELECTOR_MODAL_BULK_ACTION_FAIL

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

I don't like this name: .ez-bulk-operation-failed-modal

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

So you want all names to be changed from bulk-operation-failed to bulk-action-failed?

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Yes, it's shorter but still there's something in the class name I don't like. Can't it be even shorter by using different words describing the same element?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

Unfortunately, I was unable to come up with shorter name. 😕

@@ -1,4 +1,5 @@
(function(global, doc, $, React, ReactDOM, eZ, Routing) {
const SELECTOR_BULK_OPERATION_FAILED_MODAL = '.ez-bulk-operation-failed-modal';
const listContainers = [...doc.querySelectorAll('.ez-sil')];

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Can you convert it to: const listContainers = doc.querySelectorAll('.ez-sil');?

@@ -59,6 +60,47 @@
});
};
const generateLink = (locationId) => Routing.generate('_ezpublishLocation', { locationId });
const setModalTableTitle = (title) => {
const modalTableTitleNode = doc.querySelector(
`${SELECTOR_BULK_OPERATION_FAILED_MODAL} .ez-table-header .ez-table-header__headline`

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

It should be:

`${SELECTOR_BULK_OPERATION_FAILED_MODAL} .ez-table-header__headline`

const tableRowNode = container.querySelector('tr');

fragment.append(tableRowNode);

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

You should not do it this way. It's not performant. The append should be invoked when all the actions, for each item, in a loop are done.

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

You should care about it even if it's a Fragment;

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

I did this as it was done here: https://github.com/ezsystems/ezplatform-page-builder/blob/911eb290f55a5c98638e5518cd35d340cb7764e5/src/bundle/Resources/public/js/config-form/widgets/collection.js#L59

The append should be invoked when all the actions, for each item, in a loop are done.

I'm calling .append at the end of each iteration. I'm not sure what changes you propose.

@@ -94,6 +136,7 @@
contentTypesMap,
totalCount: subItemsList.ChildrenCount,
udwConfigBulkMoveItems,
onShowBulkOperationFailedModal: showBulkOperationFailedModal,

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Please, read it out loud: onShowBulkOperationFailedModal: showBulkOperationFailedModal. Does it make sense to you? Maybe you can simplify it and avoid a feeling of duplication?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

So, onShowBulkOperationFailedModal: showBulkOperationFailedModal -> showBulkOperationFailedModal, ?

* @param {Object} detail
* @param {String} detail.message
* @param {String} detail.label
* @param {Function} [detail.callback] Called after notification Node was added
*/
const showNotification = (detail) => {

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Ok, you're passing the callback here, but when it's invoked?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

Hmm, Called after notification Node was added from jsDoc is not precise enough?
Callback is called right after we append notification element to the DOM.

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

The thing is you're dispatching it with an event. So it may be invoked and may be not as well. What's more, I'm missing an information about the kind of this callback. Is it invoked on success or on fail?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

Is it invoked on success or on fail?

I don't know what do you mean by fail?
Could you please specify how you want it to look like? You want different name for callback, different description in jsDoc, different behaviour of the whole mechanism?

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor
  1. Please, change the description of detail.callback because it promises the callback will be invoked (but it depends on the final implementation of event listener),
  2. The callback is very generic name. You don't know what is it for. I'm missing context here.

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor
  1. I will change description to: to be called after notification Node was added, which does not tell that it will be invoked but rather that it is intended to be invoked on notification show.
  2. I think we may change name to onShow. I googled a bit and found that theres even something similar for notifications in PWA: https://developer.mozilla.org/en-US/docs/Web/API/Notification/onshow

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

I like it. Go on!

.modal-header {
background-color: $ez-color-base-pale;

.modal-title {

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Do you need to nest this selector and the one below?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

Hmm, we nest this selector in other places, but I checked it and we don't have to do this.
Also, I modeled this on notifications modal, which has separate file with CSSes styling modal, but now I realised that we are storing modals CSSes in: https://github.com/ezsystems/ezplatform-admin-ui/blob/master/src/bundle/Resources/public/scss/_modals.scss
so these styles should probably be moved to this file?

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

I think it's the right direction to put styles there.

}
}

.modal-body {

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Maybe you can use the Bootstrap utility classes instead? https://getbootstrap.com/docs/4.1/utilities/spacing/

{% trans_default_domain 'locationview' %}

<div
class="modal fade ez-bulk-operation-failed-modal"

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

Why do you duplicate id as a class name?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

I think that if we want to be compliant with BEM and use classes for elements (like ez-bulk-operation-failed-modal__table-body) we need class for whole block. I also use this class to style modal.

This comment has been minimized.

Copy link
@sunpietro

sunpietro Dec 12, 2018

Contributor

I see, then id attribute is not required, right?

This comment has been minimized.

Copy link
@tischsoic

tischsoic Dec 12, 2018

Author Contributor

I checked how it works in other places in our codebase and it seems that we have convention to give ids to modals and later refer to them by these ids. It also matches examples from https://getbootstrap.com/docs/4.0/components/modal/.

But I don't use id in my piece of code, instead I have class selector:

const SELECTOR_BULK_OPERATION_FAILED_MODAL = '.ez-bulk-operation-failed-modal';

I think that the right way would be to change above selector to:

const SELECTOR_BULK_OPERATION_FAILED_MODAL = '#bulk-operation-failed-modal'

and leave class for styling puroposes.

Are you OK with this? 🙂

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from f46509f to d95f41c Dec 12, 2018

@tischsoic tischsoic force-pushed the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch from d95f41c to e75390c Dec 12, 2018

@tischsoic

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@dew326

dew326 approved these changes Dec 13, 2018

@micszo

micszo approved these changes Dec 13, 2018

@micszo micszo added QA approved and removed Ready for review labels Dec 13, 2018

@micszo micszo removed their assignment Dec 13, 2018

@tischsoic

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

Ready for merge, should be merged with: ezsystems/ezplatform-admin-ui-modules#113

@lserwatka lserwatka merged commit ce96337 into master Dec 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@lserwatka lserwatka deleted the EZP-29747-bulk-delete-modal-with-list-of-content-items-impossible-to-delete branch Dec 13, 2018

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.