Skip to content

Commit

Permalink
refactor: subscribe to change instead of checked-changed (#8239)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vursen
vursen authored Nov 29, 2024
1 parent e421971 commit e42d3af
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 37 deletions.
37 changes: 14 additions & 23 deletions packages/grid/src/vaadin-grid-selection-column-base-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,12 @@ export const GridSelectionColumnBaseMixin = (superClass) =>

constructor() {
super();
this.__onCellTrack = this.__onCellTrack.bind(this);
this.__onCellClick = this.__onCellClick.bind(this);
this.__onCellMouseDown = this.__onCellMouseDown.bind(this);
this.__onActiveItemChanged = this.__onActiveItemChanged.bind(this);
this.__onSelectRowCheckboxChange = this.__onSelectRowCheckboxChange.bind(this);
this.__onSelectAllCheckboxChange = this.__onSelectAllCheckboxChange.bind(this);
}

/** @protected */
Expand Down Expand Up @@ -133,13 +138,11 @@ export const GridSelectionColumnBaseMixin = (superClass) =>
checkbox = document.createElement('vaadin-checkbox');
checkbox.setAttribute('aria-label', 'Select All');
checkbox.classList.add('vaadin-grid-select-all-checkbox');
checkbox.addEventListener('change', this.__onSelectAllCheckboxChange);
root.appendChild(checkbox);
// Add listener after appending, so we can skip the initial change event
checkbox.addEventListener('checked-changed', this.__onSelectAllCheckedChanged.bind(this));
}

const checked = this.__isChecked(this.selectAll, this._indeterminate);
checkbox.__rendererChecked = checked;
checkbox.checked = checked;
checkbox.hidden = this._selectAllHidden;
checkbox.indeterminate = this._indeterminate;
Expand All @@ -155,16 +158,14 @@ export const GridSelectionColumnBaseMixin = (superClass) =>
if (!checkbox) {
checkbox = document.createElement('vaadin-checkbox');
checkbox.setAttribute('aria-label', 'Select Row');
checkbox.addEventListener('change', this.__onSelectRowCheckboxChange);
root.appendChild(checkbox);
// Add listener after appending, so we can skip the initial change event
checkbox.addEventListener('checked-changed', this.__onSelectRowCheckedChanged.bind(this));
addListener(root, 'track', this.__onCellTrack.bind(this));
root.addEventListener('mousedown', this.__onCellMouseDown.bind(this));
root.addEventListener('click', this.__onCellClick.bind(this));
addListener(root, 'track', this.__onCellTrack);
root.addEventListener('mousedown', this.__onCellMouseDown);
root.addEventListener('click', this.__onCellClick);
}

checkbox.__item = item;
checkbox.__rendererChecked = selected;
checkbox.checked = selected;

const isSelectable = this._grid.__isItemSelectable(item);
Expand All @@ -178,13 +179,8 @@ export const GridSelectionColumnBaseMixin = (superClass) =>
*
* @private
*/
__onSelectAllCheckedChanged(e) {
// Skip if the state is changed by the renderer.
if (e.target.checked === e.target.__rendererChecked) {
return;
}

if (this._indeterminate || e.target.checked) {
__onSelectAllCheckboxChange(e) {
if (this._indeterminate || e.currentTarget.checked) {
this._selectAll();
} else {
this._deselectAll();
Expand All @@ -197,13 +193,8 @@ export const GridSelectionColumnBaseMixin = (superClass) =>
*
* @private
*/
__onSelectRowCheckedChanged(e) {
// Skip if the state is changed by the renderer.
if (e.target.checked === e.target.__rendererChecked) {
return;
}

this.__toggleItem(e.target.__item, e.target.checked);
__onSelectRowCheckboxChange(e) {
this.__toggleItem(e.currentTarget.__item, e.currentTarget.checked);
}

/** @private */
Expand Down
33 changes: 19 additions & 14 deletions packages/grid/test/selection.common.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from '@vaadin/chai-plugins';
import { click, fixtureSync, listenOnce, mousedown, nextFrame } from '@vaadin/testing-helpers';
import { click, fixtureSync, listenOnce, mousedown, nextFrame, nextRender } from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';
import {
Expand Down Expand Up @@ -183,7 +183,6 @@ describe('multi selection column', () => {
<vaadin-grid-filter-column path="value"></vaadin-grid-filter-column>
</vaadin-grid>
`);
await nextFrame();

grid.querySelector('[header="header"]').renderer = (root, _, { item }) => {
root.textContent = item;
Expand All @@ -205,6 +204,8 @@ describe('multi selection column', () => {

selectAllCheckbox = firstHeaderCellContent.children[0];
firstBodyCheckbox = firstBodyCellContent.children[0];

await nextRender();
});

it('should clean up listeners on detach', () => {
Expand All @@ -216,16 +217,20 @@ describe('multi selection column', () => {
expect(firstBodyCheckbox.localName).to.eql('vaadin-checkbox');
});

it('should have the checkbox unselected by default', () => {
expect(firstBodyCheckbox.checked).to.be.false;
});

it('should select item when checkbox is checked', async () => {
firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();
expect(grid._isSelected(cachedItems[0])).to.be.true;
});

it('should dispatch one event on selection', async () => {
const spy = sinon.spy();
grid.addEventListener('selected-items-changed', spy);
firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();
expect(spy.callCount).to.equal(1);
expect(spy.getCall(0).args[0].detail.value).to.equal(grid.selectedItems);
Expand Down Expand Up @@ -447,7 +452,7 @@ describe('multi selection column', () => {
});

it('should have indeterminate when an item is selected', async () => {
firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();

expect(selectAllCheckbox.indeterminate).to.be.true;
Expand All @@ -458,7 +463,7 @@ describe('multi selection column', () => {
expect(selectAllCheckbox.checked).to.be.false;
expect(selectAllCheckbox.indeterminate).not.to.be.ok;

firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();
expect(selectAllCheckbox.checked).to.be.true;
expect(selectAllCheckbox.indeterminate).to.be.true;
Expand All @@ -469,7 +474,7 @@ describe('multi selection column', () => {
await nextFrame();
expect(selectAllCheckbox.indeterminate).to.be.false;

firstBodyCheckbox.checked = false;
firstBodyCheckbox.click();
await nextFrame();
expect(selectAllCheckbox.indeterminate).to.be.true;
});
Expand All @@ -493,7 +498,7 @@ describe('multi selection column', () => {
});

it('should not have selectAll after selecting a single item', async () => {
firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();

expect(selectionColumn.selectAll).to.be.false;
Expand All @@ -503,7 +508,7 @@ describe('multi selection column', () => {
selectAllCheckbox.click();
await nextFrame();

firstBodyCheckbox.checked = false;
firstBodyCheckbox.click();
await nextFrame();

expect(selectionColumn.selectAll).to.be.false;
Expand All @@ -522,10 +527,10 @@ describe('multi selection column', () => {
it('should have selectAll after selecting all manually', async () => {
selectAllCheckbox.click();
await nextFrame();
firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();

firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();
await nextFrame();

expect(selectionColumn.selectAll).to.be.true;
Expand All @@ -536,7 +541,7 @@ describe('multi selection column', () => {
for (const row of rows) {
const cellContent = getCellContent(getRowCells(row)[0]);
const checkbox = cellContent.children[0];
checkbox.checked = true;
checkbox.click();
await nextFrame();
}

Expand All @@ -551,7 +556,7 @@ describe('multi selection column', () => {
for (const row of rows) {
const cellContent = getCellContent(getRowCells(row)[0]);
const checkbox = cellContent.children[0];
checkbox.checked = false;
checkbox.click();
await nextFrame();
}

Expand All @@ -562,7 +567,7 @@ describe('multi selection column', () => {
it('should update select all when filters change', async () => {
grid.items = [{ value: 'foo' }, { value: 'bar' }];

firstBodyCheckbox.checked = true;
firstBodyCheckbox.click();

await nextFrame();
const filter = grid.querySelector('vaadin-grid-filter');
Expand Down

0 comments on commit e42d3af

Please sign in to comment.