From e21cbf6b98cc19d153b4a4e19931b13695ae4ace Mon Sep 17 00:00:00 2001 From: Tremayne Christ Date: Wed, 17 Nov 2021 17:04:00 +0000 Subject: [PATCH 1/5] fix(tree): correctly update dependant checked states --- .../src/tree/managers/tree-manager.ts | 88 ++++++++++++------- 1 file changed, 58 insertions(+), 30 deletions(-) diff --git a/packages/elements/src/tree/managers/tree-manager.ts b/packages/elements/src/tree/managers/tree-manager.ts index a69e1666b7..1305aee8da 100644 --- a/packages/elements/src/tree/managers/tree-manager.ts +++ b/packages/elements/src/tree/managers/tree-manager.ts @@ -61,7 +61,10 @@ export class TreeManager { */ public get checkedItems (): readonly T[] { return this.composer.queryItems((item: T) => { - return this.isItemChecked(item) && (!this.manageRelationships || !this.getItemChildren(item).length); + if (this.manageRelationships && this.isItemParent(item)) { + return false; + } + return this.isItemChecked(item); }, Infinity); } @@ -131,9 +134,48 @@ export class TreeManager { * @returns `True` if the item is checked */ private isItemChecked (item: T): boolean { + if (this.manageRelationships && this.isItemParent(item)) { + return !this.getItemChildren(item).some(child => !this.isItemChecked(child)); + } return this.composer.getItemPropertyValue(item, 'selected') === true; } + /** + * Is the item checked indeterminately? + * @param item Original data item + * @returns `True` is the item has managed relationships and contains checked descendants + */ + private isItemCheckedIndeterminate (item: T): boolean { + if (this.manageRelationships && this.isItemParent(item)) { + return this.getItemDescendants(item).some(desc => this.isItemChecked(desc)); + } + return false; + } + + /** + * Determines whether the item is unchecked and can be changed to a checked state. + * @param item Original data item + * @returns True if the item can be changed to 'checked'. + */ + private canCheckItem (item: T): boolean { + if (this.manageRelationships && this.isItemParent(item)) { + return this.getItemChildren(item).some(child => this.canCheckItem(child)); + } + return this.isItemCheckable(item) && this.composer.getItemPropertyValue(item, 'selected') !== true; + } + + /** + * Determines whether the item is checked and can be changed to an unchecked state. + * @param item Original data item + * @returns True if the item can be changed to 'unchecked'. + */ + private canUncheckItem (item: T): boolean { + if (this.manageRelationships && this.isItemParent(item)) { + return this.getItemChildren(item).some(child => this.canUncheckItem(child)); + } + return this.isItemCheckable(item) && this.composer.getItemPropertyValue(item, 'selected') === true; + } + /** * Makes an item visible * @param item Original data item @@ -155,17 +197,12 @@ export class TreeManager { /** * Forces a modification event, so that the renderer can update. - * @param item Item of which to find ancestors + * @param item Item of which to find path * @returns {void} */ - private forceUpdateOnAncestors (item: T): void { - this.composer.getItemAncestors(item).forEach(ancestor => { - const allSelected = !this.getItemChildren(ancestor).some( - child => this.isItemCheckable(child) && !this.isItemChecked(child) - ); - this.composer.setItemPropertyValue(ancestor, 'selected', allSelected); - this.updateItem(ancestor); - }); + private forceUpdateOnPath (item: T): void { + const path = [...this.getItemAncestors(item), item]; + path.forEach(item => this.composer.updateItemTimestamp(item)); } /** @@ -254,16 +291,13 @@ export class TreeManager { * @returns Checked state of the item */ public getItemCheckedState (item: T): CheckedState { - const descendants = this.getItemDescendants(item).filter(descendant => !this.composer.isItemLocked(descendant)); - const isParent = descendants.length > 0; - if (isParent && this.manageRelationships) { - const checkedCount = descendants.reduce((count, item) => { - return count + (this.isItemChecked(item) === true ? 1 : 0); - }, 0); - return !checkedCount ? CheckedState.UNCHECKED - : checkedCount === descendants.length ? CheckedState.CHECKED : CheckedState.INDETERMINATE; + if (this.isItemChecked(item)) { + return CheckedState.CHECKED; + } + if (this.isItemCheckedIndeterminate(item)) { + return CheckedState.INDETERMINATE; } - return this.isItemChecked(item) === true ? CheckedState.CHECKED : CheckedState.UNCHECKED; + return CheckedState.UNCHECKED; } /** @@ -350,17 +384,14 @@ export class TreeManager { return this._checkItem(item); } private _checkItem (item: T, manageRelationships = this.manageRelationships): boolean { - if (this.isItemCheckable(item) && !this.isItemChecked(item)) { + if (this.canCheckItem(item)) { this.composer.setItemPropertyValue(item, 'selected', true); if (manageRelationships) { - this.forceUpdateOnAncestors(item); + this.forceUpdateOnPath(item); this.getItemDescendants(item).forEach(descendant => this._checkItem(descendant, false)); } return true; } - if (this.isItemParent(item)) { - this.updateItem(item); // update parent checked state - } return false; } @@ -373,17 +404,14 @@ export class TreeManager { return this._uncheckItem(item); } private _uncheckItem (item: T, manageRelationships = this.manageRelationships): boolean { - if (this.isItemCheckable(item) && this.isItemChecked(item)) { + if (this.canUncheckItem(item)) { this.composer.setItemPropertyValue(item, 'selected', false); if (manageRelationships) { - this.forceUpdateOnAncestors(item); + this.forceUpdateOnPath(item); this.getItemDescendants(item).forEach(descendant => this._uncheckItem(descendant, false)); } return true; } - if (this.isItemParent(item)) { - this.updateItem(item); // update parent checked state - } return false; } @@ -393,7 +421,7 @@ export class TreeManager { * @returns `True` if the item is modified */ public toggleItem (item: T): boolean { - return this.isItemChecked(item) ? this.uncheckItem(item) : this.checkItem(item); + return this.checkItem(item) || this.uncheckItem(item); } /** From b79b050133cea1374f8455c68d0a6a8233136117 Mon Sep 17 00:00:00 2001 From: Tremayne Christ Date: Wed, 17 Nov 2021 17:11:57 +0000 Subject: [PATCH 2/5] docs(tree): fix typo --- packages/elements/src/tree/managers/tree-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/elements/src/tree/managers/tree-manager.ts b/packages/elements/src/tree/managers/tree-manager.ts index 1305aee8da..4ea074cc68 100644 --- a/packages/elements/src/tree/managers/tree-manager.ts +++ b/packages/elements/src/tree/managers/tree-manager.ts @@ -143,7 +143,7 @@ export class TreeManager { /** * Is the item checked indeterminately? * @param item Original data item - * @returns `True` is the item has managed relationships and contains checked descendants + * @returns `True` if the item has managed relationships and contains checked descendants */ private isItemCheckedIndeterminate (item: T): boolean { if (this.manageRelationships && this.isItemParent(item)) { From 07a7cdc1334be224fa72fac1717b499e48df3fe9 Mon Sep 17 00:00:00 2001 From: Tremayne Christ Date: Thu, 18 Nov 2021 14:05:46 +0000 Subject: [PATCH 3/5] test(tree): check parent checked state --- .../elements/src/tree/__test__/tree.test.js | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/packages/elements/src/tree/__test__/tree.test.js b/packages/elements/src/tree/__test__/tree.test.js index e1cb5c97b2..dbc81bfd26 100644 --- a/packages/elements/src/tree/__test__/tree.test.js +++ b/packages/elements/src/tree/__test__/tree.test.js @@ -71,6 +71,69 @@ const nestedData = [{ value: '4' }]; +const deepNestedData = [{ + label: 'Item 1', + value: '1', + items: [ + { + label: 'Item 1.1', + value: '1.1', + }, + { + label: 'Item 1.2', + value: '1.2', + }, + { + label: 'Item 1.3', + value: '1.3', + items: [ + { + label: 'Item 1.3.1', + value: '1.3.1', + items: [ + { + label: 'Item 1.3.1.1', + value: '1.3.1.1', + selected: true, + }, + { + label: 'Item 1.3.1.2', + value: '1.3.1.2', + selected: true, + }, + { + label: 'Item 1.3.1.3', + value: '1.3.1.3', + selected: true, + }, + ], + }, + { + label: 'Item 1.3.2', + value: '1.3.2', + items: [ + { + label: 'Item 1.3.2.1', + value: '1.3.2.1', + selected: true, + }, + { + label: 'Item 1.3.2.2', + value: '1.3.2.2', + selected: true, + }, + { + label: 'Item 1.3.2.3', + value: '1.3.2.3', + selected: true, + }, + ], + }, + ], + }, + ], +}]; + describe('tree/Tree', () => { describe('Basic Tests', () => { @@ -264,6 +327,23 @@ describe('tree/Tree', () => { describe('Multiple Selection Mode', () => { + it('Shows correct checked states', async () => { + const el = await fixture(''); + el.data = deepNestedData; + await elementUpdated(el); + el.expandAll(); + await elementUpdated(el); + const item = el.children[3]; + expect(item.label).to.equal('Item 1.3'); + expect(item.checkedState).to.equal(1); // Checked + item.click(); + await elementUpdated(el); + expect(item.checkedState).to.equal(0); // Unchecked + item.nextElementSibling.click(); + await elementUpdated(el); + expect(item.checkedState).to.equal(-1); // Indeterminate + }); + it('Supports deselecting an item on tap', async () => { const el = await fixture(''); el.data = flatData; From f420a5ff0c9193eca0047e05100657efd963f2e5 Mon Sep 17 00:00:00 2001 From: Tremayne Christ Date: Thu, 18 Nov 2021 16:43:28 +0000 Subject: [PATCH 4/5] test(tree): fix IE failure --- packages/elements/src/tree/__test__/tree.test.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/elements/src/tree/__test__/tree.test.js b/packages/elements/src/tree/__test__/tree.test.js index dbc81bfd26..408ebb8a24 100644 --- a/packages/elements/src/tree/__test__/tree.test.js +++ b/packages/elements/src/tree/__test__/tree.test.js @@ -333,14 +333,18 @@ describe('tree/Tree', () => { await elementUpdated(el); el.expandAll(); await elementUpdated(el); + isIE() && await nextFrame(); const item = el.children[3]; + const itemChild = el.children[4]; expect(item.label).to.equal('Item 1.3'); expect(item.checkedState).to.equal(1); // Checked item.click(); + isIE() && await nextFrame(); await elementUpdated(el); expect(item.checkedState).to.equal(0); // Unchecked - item.nextElementSibling.click(); + itemChild.click(); await elementUpdated(el); + isIE() && await nextFrame(); expect(item.checkedState).to.equal(-1); // Indeterminate }); From f938a5e03233c5ba67c568a40d98a014a5764bf4 Mon Sep 17 00:00:00 2001 From: Tremayne Christ Date: Thu, 18 Nov 2021 16:45:36 +0000 Subject: [PATCH 5/5] test(tree): shift IE check to be consistent with others --- packages/elements/src/tree/__test__/tree.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/elements/src/tree/__test__/tree.test.js b/packages/elements/src/tree/__test__/tree.test.js index 408ebb8a24..589e0db34c 100644 --- a/packages/elements/src/tree/__test__/tree.test.js +++ b/packages/elements/src/tree/__test__/tree.test.js @@ -339,8 +339,8 @@ describe('tree/Tree', () => { expect(item.label).to.equal('Item 1.3'); expect(item.checkedState).to.equal(1); // Checked item.click(); - isIE() && await nextFrame(); await elementUpdated(el); + isIE() && await nextFrame(); expect(item.checkedState).to.equal(0); // Unchecked itemChild.click(); await elementUpdated(el);