Fix multiselect when not all rows have checkboxes#38146
Fix multiselect when not all rows have checkboxes#38146HLeithner merged 1 commit intojoomla:4.2-devfrom
Conversation
|
Looking good |
|
I have tested this item ✅ successfully on 846f1ea This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146. |
1 similar comment
|
I have tested this item ✅ successfully on 846f1ea This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146. |
|
@wilsonge can I ask for some changes? here's the edited script Script/**
* @copyright (C) 2018 Open Source Matters, Inc. <https://www.joomla.org>
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/
if (!window.Joomla) {
throw new Error('Joomla API was not properly initialised');
}
/**
* JavaScript behavior to allow shift select in administrator grids
*/
class JMultiSelect {
constructor(formElement) {
this.tableEl = document.querySelector(formElement);
if (this.tableEl) {
this.boxes = [].slice.call(this.tableEl.querySelectorAll('td input[type=checkbox]'));
this.rows = [].slice.call(document.querySelectorAll('tr[class^="row"]'));
this.checkallToggle = document.querySelector('[name="checkall-toggle"]');
this.onCheckallToggleClick = this.onCheckallToggleClick.bind(this);
this.onRowClick = this.onRowClick.bind(this);
if (this.checkallToggle) {
this.checkallToggle.addEventListener('click', this.onCheckallToggleClick);
}
if (this.rows.length) {
this.rows.forEach((row) => {
row.addEventListener('click', this.onRowClick);
});
}
}
}
// Changes the background-color on every cell inside a <tr>
// eslint-disable-next-line class-methods-use-this
changeBg(row, isChecked) {
// Check if it should add or remove the background colour
if (isChecked) {
[].slice.call(row.querySelectorAll('td, th')).forEach((elementToMark) => {
elementToMark.classList.add('row-selected');
});
} else {
[].slice.call(row.querySelectorAll('td, th')).forEach((elementToMark) => {
elementToMark.classList.remove('row-selected');
});
}
}
onCheckallToggleClick({ target }) {
const isChecked = target.checked;
this.rows.forEach((row) => {
this.changeBg(row, isChecked);
});
}
onRowClick({ target, shiftKey }) {
// Do not interfere with links or buttons
if (target.tagName && (target.tagName.toLowerCase() === 'a' || target.tagName.toLowerCase() === 'button')) {
return;
}
if (!this.boxes.length) {
return;
}
const closestRow = target.closest('tr');
const currentRowNum = this.rows.indexOf(closestRow);
const currentCheckBox = closestRow.querySelector('td input[type=checkbox]');
if (currentCheckBox) {
let isChecked = currentCheckBox.checked;
if (!(target.id === currentCheckBox.id)) {
// We will prevent selecting text to prevent artifacts
if (shiftKey) {
document.body.style['-webkit-user-select'] = 'none';
document.body.style['-moz-user-select'] = 'none';
document.body.style['-ms-user-select'] = 'none';
document.body.style['user-select'] = 'none';
}
currentCheckBox.checked = !currentCheckBox.checked;
isChecked = currentCheckBox.checked;
Joomla.isChecked(isChecked, this.tableEl.id);
}
this.changeBg(this.rows[currentRowNum], isChecked);
// Restore normality
if (shiftKey) {
document.body.style['-webkit-user-select'] = 'none';
document.body.style['-moz-user-select'] = 'none';
document.body.style['-ms-user-select'] = 'none';
document.body.style['user-select'] = 'none';
}
}
}
}
let formId = '#adminForm';
if (Joomla && Joomla.getOptions('js-multiselect', {}).formName) {
formId = `#${Joomla.getOptions('js-multiselect', {}).formName}`;
}
// eslint-disable-next-line no-new
new JMultiSelect(formId);So what's different?
A more generic comment:Since IE11 is dead, J4 didn't really supported it anyway it would be a nice to have an improvement for the tooling, eg switch the |
@dgrammatiko |
|
@richard67 just fyi the media manager had this improvement as the code is executed immediately The change was done with #30839 |
|
Let's get this in first with the bug fix - so the two can be separated if there are any issues. Out of personal interest if you defer all the js files loading - is it still in order? As in how do we know if the Joomla stuff has actually being init'd? |
|
thanks |
Pull Request for Issue #38021.
@dgrammatiko would appreciate a review here. Somehow i feel like after this populating
this.boxesis now overkill but not really sure of a good way to make that optimization after this. Feels like this is 'good enough' thoughSummary of Changes
Fixes the behaviour of multiselect when not all rows have a checkbox (because of ACL restrictions)
Testing Instructions
See reproduction steps in linked issue
Actual result BEFORE applying this Pull Request
Multiselect highlights wrong row, when not a super admin in com_users
Expected result AFTER applying this Pull Request
Multiselect highlights the right row both when super admin with full permissions and as a restricted user.
Documentation Changes Required
n/a