Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: Change VirtualScroller.renderItems strategy #900

Merged
merged 3 commits into from
Jan 24, 2019
Merged

Update: Change VirtualScroller.renderItems strategy #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86a343d
Update: Change VirtualScroller.renderItems strategy
Jan 23, 2019
6ff205a
Chore: address PR comments
Jan 23, 2019
929ba72
Chore: updating for PR comments
Jan 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 28 additions & 35 deletions src/lib/VirtualScroller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ class VirtualScroller {
newEndOffset = this.totalItems - 1;
}

// Create a new list element to be swapped out for the existing one
const newListEl = this.createListElement();
// Creates a document fragment for the new list items to be appended into the list
const fragment = document.createDocumentFragment();

if (curStartOffset <= offset && offset <= curEndOffset) {
// Scenario #1: New start offset falls within the current range of items rendered
Expand All @@ -263,77 +263,70 @@ class VirtualScroller {
// |--------------------|
// newStartOffset newEndOffset
newStartOffset = curEndOffset + 1;
// clone elements from newStartOffset to curEndOffset
this.cloneItems(newListEl, this.listEl, offset - curStartOffset, curEndOffset - curStartOffset);
// then create elements from curEnd + 1 to newEndOffset
this.createItems(newListEl, newStartOffset, newEndOffset);
// Create elements from curEnd + 1 to newEndOffset
this.createItems(fragment, newStartOffset, newEndOffset);
// Delete the elements from curStartOffset to newStartOffset
this.deleteItems(this.listEl, curStartOffset - curStartOffset, offset - curStartOffset);
// Append the document fragment to the listEl
this.listEl.appendChild(fragment);
} else if (curStartOffset <= newEndOffset && newEndOffset <= curEndOffset) {
// Scenario #2: New end offset falls within the current range of items rendered
// |--------------------|
// curStartOffset curEndOffset
// |--------------------|
// newStartOffset newEndOffset

// create elements from newStartOffset to curStart - 1
this.createItems(newListEl, offset, curStartOffset - 1);
// then clone elements from curStartOffset to newEndOffset
this.cloneItems(newListEl, this.listEl, 0, newEndOffset - curStartOffset);
// Create elements from newStartOffset to curStart - 1
this.createItems(fragment, offset, curStartOffset - 1);
// Delete the elements from newEndOffset to the end
this.deleteItems(this.listEl, newEndOffset - curStartOffset + 1);
// Insert before the firstElementChild of the listEl
this.listEl.insertBefore(fragment, this.listEl.firstElementChild);
} else {
// Scenario #3: New range has no overlap with current range of items
// |--------------------|
// curStartOffset curEndOffset
// |--------------------|
// newStartOffset newEndOffset
this.createItems(newListEl, newStartOffset, newEndOffset);
this.createItems(fragment, newStartOffset, newEndOffset);
this.listEl.appendChild(fragment);
}

this.scrollingEl.replaceChild(newListEl, this.listEl);
this.listEl = newListEl;
}

/**
* Clones a subset of the HTMLElements from the oldList to the newList.
* The newList element is modified.
* Creates new HTMLElements appended to the newList
*
* @param {HTMLElement} newListEl - the new `ol` element
* @param {HTMLElement} oldListEl - the old `ol` element
* @param {number} start - start index
* @param {number} end - end index
* @return {void}
*/
cloneItems(newListEl, oldListEl, start, end) {
if (!newListEl || !oldListEl || start < 0 || end < 0) {
return;
}

const { children } = oldListEl;

if (!children || start >= children.length || end >= children.length) {
createItems(newListEl, start, end) {
if (!newListEl || start < 0 || end < 0) {
return;
}

for (let i = start; i <= end; i++) {
newListEl.appendChild(children[i].cloneNode(true));
const newEl = this.renderItem(i);
newListEl.appendChild(newEl);
}
}

/**
* Creates new HTMLElements appended to the newList
* Deletes elements of the 'ol'
*
* @param {HTMLElement} newListEl - the new `li` element
* @param {HTMLElement} listEl - the `ol` element
* @param {number} start - start index
* @param {number} end - end index
* @param {number} [end] - end index
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should [end] be in brackets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to because if not provided (which is how I call it https://github.com/box/box-content-preview/pull/900/files#diff-ae1d3917492ce0b7a65c4b31408eb5deR282) it will just delete to the end of the listEl's children

* @return {void}
*/
createItems(newListEl, start, end) {
if (!newListEl || start < 0 || end < 0) {
deleteItems(listEl, start, end) {
if (!listEl || start < 0 || end < 0) {
return;
}

for (let i = start; i <= end; i++) {
const newEl = this.renderItem(i);
newListEl.appendChild(newEl);
}
const listItems = Array.prototype.slice.call(listEl.children, start, end);
listItems.forEach((listItem) => listEl.removeChild(listItem));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we incur a performance penalty here, since this.listEl is currently mounted? Each removeChild call will force a reflow, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so because all the li are absolutely positioned

}

/**
Expand Down
110 changes: 58 additions & 52 deletions src/lib/__tests__/VirtualScroller-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,23 +162,20 @@ describe('VirtualScroller', () => {

describe('renderItems()', () => {
let newListEl;
const curListEl = {};
let curListEl;

beforeEach(() => {
virtualScroller.scrollingEl = { replaceChild: () => {} };
stubs.appendChild = sandbox.stub();
stubs.insertBefore = sandbox.stub();
curListEl = { appendChild: stubs.appendChild, insertBefore: stubs.insertBefore };
newListEl = {};
virtualScroller.listEl = curListEl;
newListEl = { appendChild: () => {} };
stubs.createListElement = sandbox.stub(virtualScroller, 'createListElement').returns(newListEl);

stubs.renderItem = sandbox.stub(virtualScroller, 'renderItem');
stubs.replaceChild = sandbox.stub(virtualScroller.scrollingEl, 'replaceChild');
stubs.appendChild = sandbox.stub(newListEl, 'appendChild');
stubs.getCurrentListInfo = sandbox.stub(virtualScroller, 'getCurrentListInfo');
stubs.cloneItems = sandbox.stub(virtualScroller, 'cloneItems');
stubs.createItems = sandbox.stub(virtualScroller, 'createItems');
});

afterEach(() => {
virtualScroller.scrollingEl = null;
stubs.deleteItems = sandbox.stub(virtualScroller, 'deleteItems');
stubs.createDocumentFragment = sandbox.stub(document, 'createDocumentFragment').returns(newListEl);
});

it('should render the whole range of items (no reuse)', () => {
Expand All @@ -190,9 +187,10 @@ describe('VirtualScroller', () => {
});
virtualScroller.renderItems();

expect(stubs.cloneItems).not.to.be.called;
expect(stubs.deleteItems).not.to.be.called;
expect(stubs.createItems).to.be.calledWith(newListEl, 0, 10);
expect(virtualScroller.scrollingEl.replaceChild).to.be.called;
expect(stubs.appendChild).to.be.called;
expect(stubs.insertBefore).not.to.be.called;
});

it('should render the remaining items up to totalItems', () => {
Expand All @@ -204,9 +202,25 @@ describe('VirtualScroller', () => {
});
virtualScroller.renderItems(95);

expect(stubs.cloneItems).not.to.be.called;
expect(stubs.deleteItems).not.to.be.called;
expect(stubs.createItems).to.be.calledWith(newListEl, 95, 99);
expect(virtualScroller.scrollingEl.replaceChild).to.be.called;
expect(stubs.appendChild).to.be.called;
expect(stubs.insertBefore).not.to.be.called;
});

it('should render items above the current list', () => {
virtualScroller.maxRenderedItems = 10;
virtualScroller.totalItems = 100;
stubs.getCurrentListInfo.returns({
startOffset: 20,
endOffset: 30
});
virtualScroller.renderItems(15);

expect(stubs.deleteItems).to.be.called;
expect(stubs.createItems).to.be.calledWith(newListEl, 15, 19);
expect(stubs.appendChild).not.to.be.called;
expect(stubs.insertBefore).to.be.called;
});
});

Expand Down Expand Up @@ -333,60 +347,52 @@ describe('VirtualScroller', () => {
});
});

describe('cloneItems()', () => {
let newListEl;
describe('deleteItems()', () => {
let listEl;

beforeEach(() => {
stubs.appendChild = sandbox.stub();
newListEl = { appendChild: stubs.appendChild };
stubs.removeChild = sandbox.stub();
listEl = { removeChild: stubs.removeChild };
});

const paramaterizedTests = [
{ name: 'no newListEl provided', newListEl: undefined, start: 1, end: 2 },
{ name: 'no start provided', newListEl, start: undefined, end: 2 },
{ name: 'no end provided', newListEl, start: 1, end: undefined },
{ name: 'start is < 0 provided', newListEl, start: -1, end: 2 },
{ name: 'end is < 0 provided', newListEl, start: 1, end: -1 },
{ name: 'no oldListEl.children', newListEl, start: 1, end: 2 },
{
name: 'start is greater than oldListEl.children length',
newListEl,
oldListEl: { children: { length: 1 } },
start: 1,
end: 3
},
{
name: 'end is greater than oldListEl.children length',
newListEl,
oldListEl: { children: { length: 1 } },
start: 0,
end: 1
}
{ name: 'no listEl provided', listEl: undefined, start: 1, end: 2 },
{ name: 'no start provided', listEl, start: undefined, end: 2 },
{ name: 'no end provided', listEl, start: 1, end: undefined },
{ name: 'start is < 0 provided', listEl, start: -1, end: 2 },
{ name: 'end is < 0 provided', listEl, start: 1, end: -1 }
];

paramaterizedTests.forEach((testData) => {
it(`should do nothing if ${testData.name}`, () => {
const { newListEl: newList, oldListEl, start, end } = testData;
const { listEl: list, start, end } = testData;

virtualScroller.cloneItems(newList, oldListEl, start, end);
virtualScroller.deleteItems(list, start, end);

expect(stubs.appendChild).not.to.be.called;
expect(stubs.removeChild).not.to.be.called;
});
});

it('should clone over the items specified', () => {
const oldListEl = {
children: [
{ cloneNode: () => {} },
{ cloneNode: () => {} },
{ cloneNode: () => {} },
{ cloneNode: () => {} }
]
it('should remove the items specified', () => {
const list = {
children: [{}, {}, {}, {}],
removeChild: stubs.removeChild
};

virtualScroller.deleteItems(list, 0, 1);

expect(stubs.removeChild).to.be.calledOnce;
});

it('should remove the items specified from start to the end when end is not provided', () => {
const list = {
children: [{}, {}, {}, {}],
removeChild: stubs.removeChild
};

virtualScroller.cloneItems(newListEl, oldListEl, 0, 1);
virtualScroller.deleteItems(list, 2);

expect(stubs.appendChild).to.be.calledTwice;
expect(stubs.removeChild).to.be.calledTwice;
});
});

Expand Down