-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Initial addition of infinite scrolling #71
Conversation
@AStoker Very nice!!! I think the Minor feedback:
Great job man, thanks! |
Seems pretty cool. Here's another scenario to consider: What if you've got an activity list that shows the most recent activity, like a chat history. The list is scrolled to the bottom by default and new items get added but the user might want to scroll up to see historical items. This would be an infinite scroll in the opposite direction. Any thoughts on how we might handle that? |
@EisenbergEffect @AStoker Yes, that's def a use case we should support, should be pretty easy to implement from what's in this PR. But how should the public API look? What do you guys think about making this a bit more generic, instead of taking a function that gets called on bottom it could take a function that takes current index (the index of the top element this.first) as parameter, and perhaps also pass along two booleans |
That sounds reasonable. |
Provide a means to allow the user to continuously insert more data into the array being viewed. resolve: aurelia#5
bd66707
to
9a3b965
Compare
Passing those parameters in won't be a problem, and might be helpful, but I'm not sure they'd help in the situation of a reverse list. With a reverse list (like the chat log example), my guess is that you would not necessarily start at the literal "end" of an array, because then every time you add more items to the list via infinite scrolling, you'd be adding them to the beginning of the array, causing things to shift position, and that seems like it could cause all kinds of problems. |
I'm thinking that we could provide additional options like scroll direction in the binding for <li virtual-repeat.for="item of items" virtual-repeat="direction: reverse" virtual-repeat-next="getNextPage">${item}</li> For kicks and giggles, a completely hacky way of accomplishing the reverse list with minimal effort (but a reverse scroll as a consequence), you could do something like this: ul {
transform: rotate(180deg);
}
ul > li {
transform: rotate(-180deg);
} I'd rather figure a proper way of handling this, but think it might belong in a separate PR. |
@AStoker Yes, let's get this in first and then we'll take a look at the reverse list scenario. |
@AStoker Is this ready to be merged? |
@martingust, yes, should be good to go. |
@martingust, is there anything else I need to do to get this merged? |
@AStoker Nope, I'll just need to pull down and review the latest changes, will do in a little bit |
Awesome, thanks. |
@AStoker Merged. Thanks a lot for this contribution! |
Excellent! Next step for me is getting horizontal scrolling working :) Thanks for the help. |
I am currently in the process of trying to use the virtual-repeat-next attribute to implement virtual scrolling. Based on my experience with other similar implementations, and my perception of the general requirements for such an implementation, there may be some things missing from this ui-virtualization implementation. The use case I'm thinking of is this:
Item #3 is where I believe there is more support needed from the ui-virtualization implementation of virtual-repeat-next. Presumably the getMore() function will make a REST call to retrieve a specific set of new items - a specific number of items starting at a specific index. However, the params topIndex, isAtBottom, isAtTop, don't really help much with this. First, if I'm scrolling down, and I want to get the next set of appropriate items, I want to add those items to the end of the list, so the param topIndex doesn't really help with this - what's also needed is a bottomIndex. And more specifically, I would want to add onto the actual data list in the model, which presumably is larger than the viewable list; so, I'd want a way to know the current end index of the data list being used by the virtual-repeat-next. Second, when the REST call is made to retrieve more items, we'd want to know an appropriate number of items to request - i.e., a pageSize. The ui-virtualization implementation seems to know this, because from looking at the source there is an elementsInView variable that's calculated at initialization. That is the value that we'd ideally use to get new items, so that we get pages of items based on the actual visible "page size", or some factor of that. In addition, we'd probably also like to prune items from the opposite end of the list when scrolling - so, when scrolling down, we would want to prune from the top of the list. Most application implementations would likely want to do this, rather than continuously added to the model list in an unconstrained way. Without access to an elementsInView size value and both topIndex and bottomIndex values, we wouldn't know how many items to prune, and where to prune from. In general, it seems like it would be desirable to find a way to expose many of the properties of the element that is triggering the scrolling to the getMore() function - like the elementsInView, bufferSize, itemsLength, etc., properties. These are all useful to the developer who is implementing their own getMore() function for virtual scrolling. Currently, with the implementation as it stands and the limited amount of information from the topIndex, isAtBottom, isAtTop variables, it's not clear to me how to do an actual robust infite scrolling implementation. Thanks, |
A couple of additional notes - Regarding exposing the properties of the VirtualRepeat object in the ViewModel instance, one straightforward approach would be with a change like this, in virtual-repeat.js:
This would inject the VirtualRepeat object into the ViewModel bindingContext. This may not be optimal, since it then tightly couples the view into the model, and could cause some issues. Second, an additional requirement/enhancement for infinite scrolling is this - |
@don-bluelinegrid, this was definitely a first draft implementation, just to get something moving :) The parameters passed were added to help with a few specific use cases, and you're right, aren't helpful in every case. If there is a specific parameter that would be helpful as well, if you could make a PR to add that, that would be helpful. For your second point, you can define page size yourself, the virtual repeat doesn't need to know about it. It has an elementsInView variable because that is how we define how much space we have and when to get more elements. Right now that is statically calculated on element creation, but I would like to modify that so that when an element grows (a common use case), that variable will also be updated. It wouldn't be as helpful then to have the elementsInView variable correspond to your page size, because then your page size could vary throughout your use of it, and potentially cause problems. Page sizes are typically static constants (just from what I've seen/experienced, I'm not saying that they should always be that), and if that's the case, then the virtual repeat would really not have much say in that. In regards to pruning from the top of the list, I'm assuming you mean pruning from the top of the actual array, not the virtualized DOM list (since that already prunes from the top and bottom depending on which way you're scrolling). The benefit of this would be that your javascript array would not become exceptionally large. I haven't seen this kind of model before, as with todays technology, memory is cheap. A couple hundred thousand javascript elements in an array isn't going to destroy today's mobile phones or PC's. For the use case of having a Lastly, for the features you requested:
I believe some of those things might be noise and easily calculated on their own. But that being said, that's just my opinion, which is prone to being wrong ;) If you could provide a use case for each of those properties so that if they really are important, we can get them in there. TL;DRSome problems are being worked on. Pull Requests (PR) are your friend. Create issue/features for
|
Any additional feedback on this would be greatly appreciated. Right now the attribute
virtual-repeat-next
is fired whenever the virtual repeat binding gets to the bottom of the list and thinks it has the "last" element. This gives the programmer the ability to do whatever they want when they get to the end of the list, as there are multiple ways of getting "next" data.I feel a little uncomfortable creating an attribute that really does nothing (it's more of a placeholder), since it's really the virtual-repeat binding that is triggering the "getNext" functionality. Just seems a bit hacky.
I added some test cases, but more thorough tests would be appreciated.
Attempts to address #5