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.

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

fix(virtual-scroll): wrong item top calculation and visibility propagation #17345

Merged
merged 14 commits into from Mar 26, 2019

Conversation

3 participants
@DavidWiesner
Copy link
Contributor

commented Feb 1, 2019

Short description of what this resolves:

In the updateVDom a wrong index was used to set cells top position. This is only problematic when a headerFn or footerFn is provided.

I extended a unittest that will fail in the latest ionic version and describe this problem

describe('updateVDom', () => {
it('should initialize empty VDOM', () => {
const vdom: VirtualNode[] = [];
const items = [1, 2, 3, 4, 5];
const { heightIndex, cells } = mockVirtualScroll(items, () => 20,
(_, i) => i === 1 ? 'hola' : null,
(_, i) => i === 2 ? 'hola' : null
);
const range: Range = { offset: 1, length: 6 };
updateVDom(vdom, heightIndex, cells, range);
expect(vdom).toEqual([
{ cell: cells[1], change: 2, d: false, top: 20, visible: true },
{ cell: cells[2], change: 2, d: false, top: 30, visible: true },
{ cell: cells[3], change: 2, d: false, top: 50, visible: true },
{ cell: cells[4], change: 2, d: false, top: 70, visible: true },
{ cell: cells[5], change: 2, d: false, top: 80, visible: true },
{ cell: cells[6], change: 2, d: false, top: 100, visible: true }
]);
});

Detailed issue cause:
In virtual-scroll-utils.ts#L239 you are using i as overall index (including header and footer) but in virtual-scroll-utils.ts#L49 you are using the raw item index (excluding header and footer).

Also if the approxHeaderHeight perfectly matches the calculated height of the node, the visibility of the cell was not propagated to the dom.
In virtual-scroll.tsx#L330 you update the visibility of the item, but this update will only scheduled, when the height is different to the previous known height.

Changes proposed in this pull request:

  • use the right index in updateVDom to update the top transition ()
  • extend unit test to verify the top is also calculated right with a given headerFn and footerFn
  • update the visibility of the node also if a given approxHeaderHeight/approxFooterHeight matches the calculated height

Ionic Version:
4.0.2
Fixes: #15948, #17298

@ionitron-bot ionitron-bot bot added the package: core label Feb 1, 2019

@DavidWiesner DavidWiesner force-pushed the DavidWiesner:issue-15948 branch from d11069f to 827cfb2 Feb 4, 2019

@DavidWiesner DavidWiesner changed the title fix VirtualScroll itemHeight bug #15948 fix(virtual-scroll): wrong item top calculation and visibility propagation Feb 4, 2019

@DavidWiesner DavidWiesner referenced this pull request Feb 4, 2019

Open

todo(ion-virtual-scroll): Bugs and feature requests #16632

6 of 20 tasks complete

DavidWiesner and others added some commits Feb 5, 2019

@DavidWiesner

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

You guys did a great job by rewriting virtual scroll. Both issues solved in this pr are important to fix. My company spend a lot of effort to port our three apps to ionic 4. Is there any reason why this pr will not get merged? What I can help to get this merged? Should I write another unittest or add a new unittest instead of replacing the old? Thx

DavidWiesner and others added some commits Mar 5, 2019

@DavidWiesner

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I add some more details about the causes of the issues. To help the reviewers understanding my changes.

brandyscarney and others added some commits Mar 14, 2019

@liamdebeasi
Copy link
Member

left a comment

Thanks for the PR! 🎉

brandyscarney added some commits Mar 19, 2019

@brandyscarney brandyscarney merged commit a8a48a4 into ionic-team:master Mar 26, 2019

1 check passed

build Workflow: build
Details
@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Thanks @DavidWiesner! I'm going to release a dev build and I'll add it to those issues for testing.

@brandyscarney brandyscarney added this to In progress 🤺 in Ionic Core via automation Mar 26, 2019

@liamdebeasi liamdebeasi moved this from In progress 🤺 to Done 🎉 in Ionic Core Mar 28, 2019

Kiku-git added a commit to Kiku-git/ionic that referenced this pull request May 16, 2019

fix(virtual-scroll): use correct item top calculation with header or …
…footer function (ionic-team#15948) (ionic-team#17345)

- use the right index in updateVDom to update the top transition ()
- extend unit test to verify the top is also calculated right with a given headerFn and footerFn
- update the visibility of the node also if a given approxHeaderHeight/approxFooterHeight matches the calculated height

fixes ionic-team#15948 fixes ionic-team#17298
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.