-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(virtualRepeat): Infinite scroll and deferred data loading #4002
Conversation
// Using a mixin works too. All that matters | ||
// is that we inherit from mdVirtualRepeatModel and implement | ||
// getItemAtIndex and getLength. | ||
this.infiniteItems = Object.create(mdVirtualRepeatModel.prototype); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed here ?
6ee8cd4
to
1dfd33f
Compare
explicitly use <code>md-virtual-repeat-container</code> as a wrapping parent container. | ||
<br/><br/> | ||
To enable load-on-demand behavior, developers must pass in a custom instance of | ||
mdVirtualRepeatModel (see the example's source for more info). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking it would be easier for the user to instead explicitly set some attribute when they want infinite scrolling and then check that the model provided has the appropriate methods. Something like:
<div md-virtual-repeat="item in ctrl.dynamicItems" md-infinite>
{{item}}
</div>
I think most angular users would rather specify an attribute than extend a base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasBurleson Thoughts? That would be less Google JSish, but might also result in less useful errors if they don't implement the interface methods (unless I manually check and throw exceptions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say to manually check and throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, md-infinite isn't quite right... infinite scroll is one of two behaviors unlocked by this feature (hence the two demos).
md-deferred? md-deferred-data? md-deferred-load?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
md-on-demand
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jelbourn, @kseamon - I like the md-on-demand
attribute and the use of the attribute instead of the model subclassing. Currently issues:
- I do not think we are clearly articulating the expected model interface methods when
onDemand
is enabled; the comment is very easy to miss. - Should this information also be in the source docs?
- If the interface is not correct, I think we should throw an Error or at least warn the developer in the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is enforced by exceptions and is documented in the virtual repeat source code.
1dfd33f
to
d4f31b7
Compare
|
||
function VirtualRepeatModelArrayLike(model) { | ||
this.model = model; | ||
this.length = model.getLength(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check here whether model
has the required functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I'd rather not have to do these checks on each digest cycle, but for full correctness I'd have to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a one-time check is sufficient for now.
LGTM for the code itself, would be good to hear Thomas' thoughts on the attribute name. |
demo works dynamic loading demo WiP. comments comment Add a comment for method. jeremy's suggestion exception
d4f31b7
to
eb8ad64
Compare
for (var i = pageOffset; i < pageOffset + this.PAGE_SIZE; i++) { | ||
this.loadedPages[pageNumber].push(i); | ||
} | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience the following is always a better approach:
var buffer = [], pageOffset = pageNumber * this.PAGE_SIZE;
for (var i = pageOffset; i < pageOffset + this.PAGE_SIZE; i++) {
buffer.push(i);
}
this.loadedPages[pageNumber] = buffer;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better how? Remember this is just a demo script.
@kseamon - really cool feature here. |
What's blocking merging of this PR at this point? |
@ThomasBurleson added the "needs work" label, but I don't know what he had in mind. |
@kseamon, @jelbourn - On the local source (with this PR merged), I just updated the source with additional comments to clearly articulate the use of the required interface: /**
* This VirtualRepeatModelArrayLike class enforces the interface requirements
* for infinite scrolling within a mdVirtualRepeatContainer. An object with this
* interface must implement the following interface with two (2) methods:
*
* getItemAtIndex: function(index) -> item at that index or null if it is not yet
* loaded (It should start downloading the item in that case).
*
* getLength: function() -> number The data legnth to which the repeater container
* should be sized. Ideally, when the count is known, this method should return it.
* Otherwise, return a higher number than the currently loaded items to produce an
* infinite-scroll behavior.
*
* @usage
* <hljs lang="html">
* <md-virtual-repeat-container md-orient-horizontal>
* <div md-virtual-repeat="i in items" md-on-demand>
* Hello {{i}}!
* </div>
* </md-virtual-repeat-container>
* </hljs>
*
*/
function VirtualRepeatModelArrayLike(model) {
if (!angular.isFunction(model.getItemAtIndex) ||
!angular.isFunction(model.getLength)) {
throw Error('Object passed to md-virtual-repeat must implement functions getItemAtIndex() ' +
'and getLength() when md-on-demand is enabled');
}
this.model = model;
} |
I am merging now. |
includes demos for both infinite scroll and deferred loading. Closes angular#4002.
Includes demos for both infinite scroll and deferred loading.
Could be enhanced further in the future with a loading spinner of some sort.