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

Added ability to use virtual scroll with different heights of elements #206

Closed
wants to merge 22 commits into from

Conversation

speige
Copy link
Collaborator

@speige speige commented Jul 20, 2018

No description provided.

@speige
Copy link
Collaborator Author

speige commented Jul 20, 2018

This has been merged. However, I haven't finished testing yet & won't publish to npm until I'm confident it's working.

The demo app doesn't have a differing height example to test with. But, I'm currently only concerned about regressions with same size items that were working before.

Also, because the code is more complex for size calculations there could be a performance difference. It's not noticeable, but it might be worth adding a flag to short-circuit the more complex logic if the consuming application developer knows it's not needed.

The examples in the demo work 99% the same as before, but I've noticed two issues.

#1) On the "Table" demo the scroll bar acts funny when it gets to the very end of the list. The scroll bar size changes and the items jump around a little.

#2) On the "Loading In Chunks" demo the end event is fired 99% of the time, but randomly doesn't fire sometimes. If you scroll away and back to the bottom it will fire the 2nd time.

@speige
Copy link
Collaborator Author

speige commented Jul 20, 2018

Git doesn't recognize it as merged because there were many merge conflicts to resolve. The original contributors may want to review the merged code to ensure nothing was missed.

@speige
Copy link
Collaborator Author

speige commented Jul 20, 2018

I've noticed that some of the examples in the demo page of https://github.com/kykint/ngx-virtual-scroll-plus are buggy or broken. This leads me to believe that the original code was not quite ready for a PR yet. Either way, it's already merged & I'm working to track down the last few bugs before closing the PR.

@speige
Copy link
Collaborator Author

speige commented Jul 21, 2018

I'm putting this feature behind an "enableUnequalChildrenSizes_Experimental" flag for now. When it becomes more stable I'll remove the _Experimental. I may eventually remove the flag entirely, but want it to stay opt-in for now since many users have fixed-size children & don't need the additional overhead.

@speige
Copy link
Collaborator Author

speige commented Jul 21, 2018

Will close this PR once we have a working demo of variable heights. Assuming the merge conflicts were resolved correctly this should be fairly simple, but I won't be 100% confident the PR was successful until I see a demo which works identical between this repo & kykint/ngx-virtual-scroll-plus.

@speige
Copy link
Collaborator Author

speige commented Jul 21, 2018

I did a lot of refactoring & added a few new features & completely broke the variable heights. Will work on correcting soon.

@speige
Copy link
Collaborator Author

speige commented Jul 23, 2018

Ok. I created an example of variable sized elements in the demo folder. It uses random width/height items. I tested it against https://github.com/kykint/ngx-virtual-scroll-plus (master branch) and got poor results. This makes me believe that the PR isn't quite ready (or I'm doing something wrong).

Instead of trying to make the merged PR work, I instead extracted a small part of it (from calculateDimensions) & deleted everything else (from calculateItems). The solution turned out very simple/elegant, & surprisingly works pretty well (but still not 100% perfect). The main issue is that the items jump a few pixels as you scroll. I wonder if I resurrect & modify the other portion of the PR that I deleted if the slight jumpiness will go away.

Although the jumpiness might be able to be improved, I'm hoping it's "close-enough".
I'm going to close this PR. I'd like to give users some time to test it & provide feedback. It's available on npm.

If anyone has ideas about further improvements to the variable-height system, let me know. I'm happy to look at future PRs if anyone makes further improvements.

My guess is the portion that I deleted from the PR which calculates the padding could be helpful in reducing the jitter. I'll let someone else look into that if they'd like.

@speige speige closed this Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants