EZP-29089: Usability bug when deleting containers #704
Conversation
This comment has been minimized.
This comment has been minimized.
ViniTou
requested review from
mikadamczyk,
dew326,
sunpietro,
tischsoic and
adamwojs
and removed request for
dew326
| }; | ||
|
|
||
| toggleForm.addEventListener('submit', openTrashImageAssetModal, false); | ||
|
|
| { | ||
| $children = $this->locationService->loadLocationChildren($item); | ||
|
|
||
| return 0 !== $children->totalCount; |
There was a problem hiding this comment.
maybe 0 < $children->totalCount
| @@ -0,0 +1,17 @@ | |||
| (function (global, doc) { | |||
| const toggleForm = doc.querySelector('form[name="location_trash_container"]'); | |||
| const haveAsset = toggleForm.dataset.haveAsset; | |||
| (function (global, doc) { | ||
| const toggleForm = doc.querySelector('form[name="location_trash_container"]'); | ||
| const haveAsset = toggleForm.dataset.haveAsset; | ||
| const haveUniqueAsset = toggleForm.dataset.haveUniqueAsset; |
There was a problem hiding this comment.
should we not be consistent with how the corresponding class is named?
| @@ -0,0 +1,27 @@ | |||
| (function (global, doc) { | |||
| const toggleForms = [...doc.querySelectorAll('.ez-toggle-btn-state-checkbox')]; | |||
There was a problem hiding this comment.
Due to latest changes in the browsers specification implementation, doc.querySelectorAll('.ez-toggle-btn-state-checkbox') is just enough. No need to convert it to an array to use with forEach only.
| const ANY_CHECKED = 'any-checked'; | ||
|
|
||
| toggleForms.forEach((toggleForm) => { | ||
| const checkboxInputs = [...toggleForm.querySelectorAll('input[type="checkbox"]')]; |
|
|
||
| toggleForms.forEach((toggleForm) => { | ||
| const checkboxInputs = [...toggleForm.querySelectorAll('input[type="checkbox"]')]; | ||
| const toggleButtonState = () => { |
There was a problem hiding this comment.
This function should be implemented outside of forEach loop. If you want to have an access to the checkboxInputs then a useful piece of information is that a forEach loop callback takes 3 params: item, index, an iterated array with all values. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
| const toggleButtonState = () => { | ||
| const button = doc.querySelector(toggleForm.dataset.toggleButtonId); | ||
|
|
||
| const anyIsChecked = checkboxInputs.some(el => el.checked); |
There was a problem hiding this comment.
CS: all variables should come one after another without additional empty lines between them.
| {{ form_widget(form.trashContainer) }} | ||
| </div> | ||
| <div class="modal-footer"> | ||
| <button type="button" class="btn btn-secondary btn--no" data-dismiss="modal"> |
There was a problem hiding this comment.
According to our style guidelines (https://doc.ezplatform.com/en/2.2/guidelines/components/buttons/) this button probably should have classes btn btn-dark.
|
|
||
| const toggleButtonState = (toggleForm, checkboxInputs) => { | ||
| const button = doc.querySelector(toggleForm.dataset.toggleButtonId); | ||
| const anyIsChecked = checkboxInputs.some(el => el.checked); |
| const checkboxInputs = toggleForm.querySelectorAll('input[type="checkbox"]'); | ||
|
|
||
| checkboxInputs.forEach((input, index, inputs) => { | ||
| input.addEventListener('change', toggleButtonState.bind(null, toggleForm, [...inputs]), false); |
There was a problem hiding this comment.
Do you need [...inputs]?
There was a problem hiding this comment.
I can't use .some and .every on Node Collection, unless there is some other way to do this.
| const toggleButtonState = (toggleForm, checkboxInputs) => { | ||
| const button = doc.querySelector(toggleForm.dataset.toggleButtonId); | ||
| const anyIsChecked = checkboxInputs.some(el => el.checked); | ||
| const allChecked = checkboxInputs.every(el => el.checked); |
There was a problem hiding this comment.
I think this line and the line above can be moved into the line line no. 22 and 23, for better performance.
|
@mnocon Behat is failing here |
|
Perhaps rebase is needed with master after your fix will be merged up. |
|
@ViniTou could you rebase with master? |
Description
This checks if location is non-empty container and if it is displays additional info modal. If content have asset fieldtype additional modals are displayed after first one.
Checklist:
$ composer fix-cs)