Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(virtualRepeat): Infinite scroll and deferred data loading #4002

Closed
wants to merge 1 commit into from

Conversation

kseamon
Copy link
Contributor

@kseamon kseamon commented Aug 3, 2015

Includes demos for both infinite scroll and deferred loading.
Could be enhanced further in the future with a loading spinner of some sort.

// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed here ?

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).
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

md-on-demand ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

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.

Copy link
Contributor Author

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.


function VirtualRepeatModelArrayLike(model) {
this.model = model;
this.length = model.getLength();
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

@jelbourn
Copy link
Member

jelbourn commented Aug 5, 2015

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
@kseamon kseamon force-pushed the virtual-infinite-model branch from d4f31b7 to eb8ad64 Compare August 5, 2015 21:52
for (var i = pageOffset; i < pageOffset + this.PAGE_SIZE; i++) {
this.loadedPages[pageNumber].push(i);
}
}));
Copy link
Contributor

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;

Copy link
Contributor Author

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.

@ThomasBurleson
Copy link
Contributor

@kseamon - really cool feature here.

@ThomasBurleson ThomasBurleson self-assigned this Aug 13, 2015
@ThomasBurleson ThomasBurleson added this to the 0.11.0 milestone Aug 13, 2015
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 13, 2015
@ThomasBurleson ThomasBurleson modified the milestone: 0.11.0 Aug 14, 2015
@naomiblack naomiblack modified the milestones: 0.12.0, 0.11.0 Aug 14, 2015
@kseamon
Copy link
Contributor Author

kseamon commented Aug 17, 2015

What's blocking merging of this PR at this point?

@jelbourn
Copy link
Member

@ThomasBurleson added the "needs work" label, but I don't know what he had in mind.

@ThomasBurleson
Copy link
Contributor

@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;
}

@ThomasBurleson
Copy link
Contributor

I am merging now.

@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Aug 18, 2015
@kseamon kseamon closed this in d68b4f6 Aug 18, 2015
@ThomasBurleson ThomasBurleson removed the pr: merge ready This PR is ready for a caretaker to review label Aug 18, 2015
kennethcachia pushed a commit to kennethcachia/material that referenced this pull request Sep 23, 2015
includes demos for both infinite scroll and deferred loading.

Closes angular#4002.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants