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

Initial addition of infinite scrolling #71

Merged
merged 8 commits into from
Jul 8, 2016

Conversation

AStoker
Copy link
Contributor

@AStoker AStoker commented Jul 1, 2016

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

@martingust
Copy link
Contributor

@AStoker Very nice!!! I think the virtual-repeat-next attribute is fine, similar approach is used in the i18n library with the t-params attribute.

Minor feedback:

  • Update commit message to follow our Git Commit Guidelines
  • If possible split up in different commits: feat, chore (packages updates) etc
  • Don't include dist files - they should only be generated with a release
  • Check gulp lint
  • More tests if you think it's needed?

Great job man, thanks!

@EisenbergEffect
Copy link
Contributor

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?

@martingust
Copy link
Contributor

@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 isAtTop and isAtBottom. That should give the user more control and flexibility to load when at bottom, top or even somewhere in between as asked about here. Thoughts?

@EisenbergEffect
Copy link
Contributor

That sounds reasonable.

Provide a means to allow the user to continuously insert more data into the array being viewed.

resolve: aurelia#5
@AStoker
Copy link
Contributor Author

AStoker commented Jul 5, 2016

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 think there needs to be some way to still have the array in a logical order, but tweak the display logic to make this happen.
Should that feature be a separate PR @martingust, or should it be included in this?

@AStoker
Copy link
Contributor Author

AStoker commented Jul 5, 2016

I'm thinking that we could provide additional options like scroll direction in the binding for virtual-repeat. So the syntax would look something like this:

<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.

@martingust
Copy link
Contributor

@AStoker Yes, let's get this in first and then we'll take a look at the reverse list scenario.

@martingust
Copy link
Contributor

@AStoker Is this ready to be merged?

@AStoker
Copy link
Contributor Author

AStoker commented Jul 6, 2016

@martingust, yes, should be good to go.

@AStoker
Copy link
Contributor Author

AStoker commented Jul 8, 2016

@martingust, is there anything else I need to do to get this merged?

@martingust
Copy link
Contributor

@AStoker Nope, I'll just need to pull down and review the latest changes, will do in a little bit

@AStoker
Copy link
Contributor Author

AStoker commented Jul 8, 2016

Awesome, thanks.

@martingust martingust merged commit 45f74be into aurelia:master Jul 8, 2016
@martingust
Copy link
Contributor

@AStoker Merged. Thanks a lot for this contribution!

@AStoker
Copy link
Contributor Author

AStoker commented Jul 8, 2016

Excellent! Next step for me is getting horizontal scrolling working :) Thanks for the help.

@don-bluelinegrid
Copy link

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:

  1. An initial set of items is retrieved from a backend, and displayed in the view. Based on the view layout, the visible list length is 15.
  2. User scrolls the list, and at some point the ui-virtualization implementation triggers a call to getMore(topIndex, isAtBottom, isAtTop).
  3. The aurelia handler for getMore() should now retrieve an appropriate new set of data to add to the list.

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,
Don

@don-bluelinegrid
Copy link

don-bluelinegrid commented Aug 5, 2016

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:

VirtualRepeat.prototype.bind = function bind(bindingContext, overrideContext) {
this.scope = { bindingContext: bindingContext, overrideContext: overrideContext };
bindingContext._virtualRepeat = this; <==
};

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 -
99% of RESTful GET requests return a 'totalCount' property with the response. This property is frequently used by infinite-scroll implementations to control the position of the scrollbar thumb. The totalCount and current topIndex provide enough information to position the scroll thumb in a proper relative position based on the items currently in view. I see that the virtual repeat template implementation uses a clever approach of a pair of 'dummy' items before ad after the visible list whose heights are managed by the component; making use of the 'totalCount' property to calculate those heights should make it possible for the scroll thumb to appear in a correct relative position, rather than it scrolling fully to the bottom when there are actually more items in the total list.

@AStoker
Copy link
Contributor Author

AStoker commented Aug 9, 2016

<book>

@don-bluelinegrid, this was definitely a first draft implementation, just to get something moving :)
That being said, I'm now going to revisit it to try and resolve a few issues that came up when we started implementing this.
Your use case that you gave for the getMore function is the one I'm going to hit right now, as that is what we found. If you start with a small page size, then the scroll function that gets more from the virtual list isn't fired (since they're already all showing), therefore the getMore function isn't fired. A temporary resolution for this is to increase your page size for the amount of items to get, but that isn't a fix, it's a bandaid. So I'm going to start working on that.

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 adding onto the end of your data list, you should be able to do that now, if I understand you correctly. If you're scrolling through 100 items let's say, and you're at the point where your last index is 50 and you're getting more, well that's all the information you need. When the getMore function is called, you know the last index since that is the last item in your array (the getMore function is really just a callback that allows you to add more items). So if that doesn't answer that use case, perhaps I don't understand it well enough, and if you could elaborate more, that would be great.

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.
More so, if you start pruning from the top of the list, then you are going to start running into problems with having the appropriate buffer size for your scroll bars (although this is just a technical hurdle that could be jumped). But the real downside of this is that you will be making two requests for the same data if the user is scrolling down and then back up. Requests for data that we already have is not a very good idea if you have a system with even just a couple hundred users. You'll see that most applications and websites implement cache for this very reason, we don't want to make duplicate requests for data we've already seen. So pruning off your data model from the top or bottom, you should be able to do, but from the use cases I'm hearing, would be bad practice.

For the use case of having a totalCount property from a response, I agree, it would be very helpful to have an additional property that we could pass in that gives the virtual repeat binding better scroll locations. If there isn't already, it would be nice to have that tracked as an issue/feature so that if you or anybody else makes a PR for it, everybody would benefit.

Lastly, for the features you requested:

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.

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;DR

Some problems are being worked on. Pull Requests (PR) are your friend. Create issue/features for totalCount behavior.

</book>

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.

4 participants