From 81d06d211a81335e79da0a63150183e5c9a03568 Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Tue, 26 Mar 2019 15:19:13 +0200 Subject: [PATCH 1/6] (tests) add cosmoz-data-nav spec --- test/.eslintrc.json | 3 + test/helpers/utils.js | 11 +++ test/index.html | 3 +- test/spec.html | 225 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 test/helpers/utils.js create mode 100644 test/spec.html diff --git a/test/.eslintrc.json b/test/.eslintrc.json index 328c2ce..0c6b7b9 100644 --- a/test/.eslintrc.json +++ b/test/.eslintrc.json @@ -13,5 +13,8 @@ "MockInteractions": false, "sinon": true, "WCT": false + }, + "rules": { + "no-unused-expressions": "off" } } diff --git a/test/helpers/utils.js b/test/helpers/utils.js new file mode 100644 index 0000000..99a62db --- /dev/null +++ b/test/helpers/utils.js @@ -0,0 +1,11 @@ +(() => { + const flushRenderQueue = nav => { + while (nav._indexRenderQueue.length) { + nav._renderQueue(); + } + }; + + Object.assign(window, { + flushRenderQueue + }); +})(); diff --git a/test/index.html b/test/index.html index 89aaea2..a5f016e 100644 --- a/test/index.html +++ b/test/index.html @@ -9,7 +9,8 @@ diff --git a/test/spec.html b/test/spec.html new file mode 100644 index 0000000..a2474de --- /dev/null +++ b/test/spec.html @@ -0,0 +1,225 @@ + + + + + cosmoz-data-nav-basic + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 9afafd4e30cf7f47b83e470e36f3ffd5ab969c81 Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Tue, 26 Mar 2019 13:32:32 +0200 Subject: [PATCH 2/6] (tests) add failing tests for maintainSelection --- test/spec.html | 74 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/test/spec.html b/test/spec.html index a2474de..328b6b1 100644 --- a/test/spec.html +++ b/test/spec.html @@ -39,6 +39,7 @@ @@ -146,7 +147,78 @@ }); describe('maintainSelection', () => { - it('controls whether the selection should be maintained when `items` change'); + context('when it is false', () => { + it('changes to `items` reset `selected` to 0', () => { + nav.items = [{id: 'a'}, {id: 'b'}, {id: 'c'}]; + nav.selected = 1; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 1'); + + nav.items = [{id: 'c'}, {id: 'd'}, {id: 'e'}]; + expect(nav.selected).to.equal(0); + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: c,index: 0'); + }); + }); + + context('when it is true', () => { + beforeEach(() => { + nav.maintainSelection = true; + }); + + it('on `items` change, updates `selected` to match the last selected item, by reference', () => { + const item = {id: 'b', data: 'somedata'}; + nav.items = [{id: 'a'}, item, {id: 'c'}]; + nav.selected = 1; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 1somedata'); + + item.data = 'otherdata'; + nav.items = [{id: 'a'}, {id: 'd'}, {id: 'e'}, item]; + expect(nav.selected).to.equal(3); + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 3otherdata'); + }); + + it('on `items` change, updates `selected` to match the last selected item, by id', () => { + nav.items = [{id: 'a'}, {id: 'b', data: 'somedata'}, {id: 'c'}]; + nav.selected = 1; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 1somedata'); + + nav.items = [{id: 'a'}, {id: 'd'}, {id: 'e'}, {id: 'b', data: 'otherdata'}]; + expect(nav.selected).to.equal(3); + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 3otherdata'); + }); + + it('on `items` change, maintains `selected` to it\'s current value, if the last selected item is no longer present, by reference or by id', () => { + nav.items = [{id: 'a'}, {id: 'b'}, {id: 'c'}]; + nav.selected = 1; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: b,index: 1'); + + nav.items = [{id: 'a'}, {id: 'd'}, {id: 'e'}]; + expect(nav.selected).to.equal(1); + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: d,index: 1'); + }); + + context('when idPath is set', () => { + it('works as expected', () => { + nav.idPath = 'deep.id'; + nav.items = [{deep: {id: 'a'}}, {deep: {id: 'b'}, data: 'somedata'}, {deep: {id: 'c'}}]; + nav.selected = 1; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: ,index: 1somedata'); + + nav.items = [{deep: {id: 'a'}}, {deep: {id: 'd'}}, {deep: {id: 'e'}}, {deep: {id: 'b'}, data: 'otherdata'}]; + expect(nav.selected).to.equal(3); + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: ,index: 3otherdata'); + }); + }); + }); }); describe('preload', () => { From e01ce01fbd8dd0e405ee9fb85f720a03d227d157 Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Tue, 26 Mar 2019 15:18:44 +0200 Subject: [PATCH 3/6] (tests) fix test that calls done multiple times --- test/basic.html | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/basic.html b/test/basic.html index 0984573..aa7ae92 100644 --- a/test/basic.html +++ b/test/basic.html @@ -339,10 +339,12 @@ assert.isTrue(nav.animating); assert.equal(nav.selected, 1); // wait for animation to end and check animating is false - nav.addEventListener('transitionend', () => { + const once = () => { + nav.removeEventListener('transitionend', once); assert.isFalse(nav.animating); done(); - }); + }; + nav.addEventListener('transitionend', once); }); }); From 5e2d92d9f4dc1274f94c6920ebdd8bb1f644189b Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Wed, 27 Mar 2019 08:48:47 +0200 Subject: [PATCH 4/6] (minor) fix maintainSelection implementation When maintainSelection is true, data-nav should always try to update the index to match the previously selected item in the new items array. --- cosmoz-data-nav.js | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/cosmoz-data-nav.js b/cosmoz-data-nav.js index 3b52746..8812e77 100644 --- a/cosmoz-data-nav.js +++ b/cosmoz-data-nav.js @@ -240,6 +240,7 @@ constructor() { super(); + this._previouslySelectedItem = null; this._cache = {}; this._preloadIdx = 0; this._boundOnTemplatesChange = this._onTemplatesChange.bind(this); @@ -268,6 +269,7 @@ this._selectDebouncer.cancel(); } + this._previouslySelectedItem = null; this._cache = {}; this._indexRenderQueue = []; window.removeEventListener('cosmoz-cache-purge', this._onCachePurgeHandler); @@ -445,10 +447,11 @@ _itemsChanged(items) { const length = items && items.length; - //Update readOnly queueLength + // update read-only properties this._setQueueLength(length >> 0); this._setHasItems(!!length); + // replace incomplete items with cached item data if (length) { items.forEach((item, index) => { if (this.isIncompleteFn(item) && this._cache[item]) { @@ -457,22 +460,31 @@ }); } + // synchronize `selected` with hash params if (this._updateSelectedFromHash()) { return; } + // reset queue to 0 or maintain selection let index = 0; - if (this.maintainSelection && this.selected > 0) { - const selectedId = this._getItemId(this.selectedItem); - index = Math.max(0, items.findIndex(item => this._getItemId(item) === selectedId)); - } + if (this.maintainSelection && this._previouslySelectedItem != null) { + // search for previously selected item by reference + index = items.indexOf(this._previouslySelectedItem); + + // if not found, search by id + if (index === -1) { + const prevId = this._getItemId(this._previouslySelectedItem); + index = items.findIndex(item => this._getItemId(item) === prevId); + } - if (this.selected === index) { - return this._updateSelected(); + // if still not found, remain on the selected index, but force re-render + if (index === -1) { + return this._updateSelected(); + } } - this.selected = index; - + // update selected or force re-render if selected did not change + return this.selected === index ? this._updateSelected() : this.selected = index; } /** @@ -487,6 +499,7 @@ const position = selected < this.items.length ? selected : selected - 1; this._setSelectedNext((position || 0) + 1); this._preload(position); + this._previouslySelectedItem = this.items[position]; const element = this._getElement(position); From 641a56770a8a7746a9be2d610341353aec48825a Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Tue, 26 Mar 2019 15:30:30 +0200 Subject: [PATCH 5/6] (patch) _forwardItem: sending props to constructor should be faster than assigning after instancing --- cosmoz-data-nav.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cosmoz-data-nav.js b/cosmoz-data-nav.js index 8812e77..7fa3ec1 100644 --- a/cosmoz-data-nav.js +++ b/cosmoz-data-nav.js @@ -854,8 +854,8 @@ if (Polymer.flush) { Polymer.flush(); } - const instance = new this._elementCtor({}); - Object.assign(instance, { [this.as]: item }, this._getBaseProps(idx)); + const props = Object.assign({ [this.as]: item }, this._getBaseProps(idx)); + const instance = new this._elementCtor(props); element.__instance = instance; element.item = item; From b5030f9dc6f5f01834a4c555dc1d4b95027d92ae Mon Sep 17 00:00:00 2001 From: Cristian Necula Date: Wed, 27 Mar 2019 10:56:58 +0200 Subject: [PATCH 6/6] (tests) specify behavior when an item changes --- test/spec.html | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/spec.html b/test/spec.html index 328b6b1..8de7fc6 100644 --- a/test/spec.html +++ b/test/spec.html @@ -144,6 +144,47 @@ expect(nav.querySelector('div.selected').textContent).to.equal('id: a,index: 0'); }); + + it('does not update the view when an item changes', () => { + const item = {id: 'a', data: 'somedata'}; + nav.items = [item]; + flushRenderQueue(nav); + expect(nav.querySelector('div.selected').textContent).to.equal('id: a,index: 0somedata'); + + // attempt #1: updating a reference directly does not update the view + // this is expected as the data-nav has no way of knowing that data was updated + item.data = 'newdata'; + expect(nav.items[0]).to.have.property('data', 'newdata'); + + expect(() => { + expect(nav.querySelector('div.selected').textContent).to.equal('id: a,index: 0newdata'); + }).throws('expected \'id: a,index: 0somedata\' to equal \'id: a,index: 0newdata\''); + + + // attempt #2: modify item data using polymer manipulation features + // this could work, but is not implemented + // an observer for items.* should forward the path update down to the rendered template instances + nav.set('items.0.data', 'otherdata'); + expect(nav.items[0]).to.have.property('data', 'otherdata'); + + expect(() => { + expect(nav.querySelector('div.selected').textContent).to.equal('id: a,index: 0otherdata'); + }).throws('expected \'id: a,index: 0somedata\' to equal \'id: a,index: 0otherdata\''); + + // attempt #3: try to force render + // this does not work, because data-nav checks if it should re-render + // the template using a reference check. Because the already rendered + // item is the same by reference, the view is not updated. + item.data = 'freshdata'; + expect(nav.items[0]).to.have.property('data', 'freshdata'); + + nav._updateSelected(); + flushRenderQueue(nav); + + expect(() => { + expect(nav.querySelector('div.selected').textContent).to.equal('id: a,index: 0freshdata'); + }).throws('expected \'id: a,index: 0somedata\' to equal \'id: a,index: 0freshdata\''); + }); }); describe('maintainSelection', () => {