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

Fixes #1058 - Fetching and rendering all comments #1119

Merged
merged 2 commits into from
Jul 14, 2016

Conversation

deepthivenkat
Copy link
Member

  1. Fetch and Render page 1 (30 comments)
  2. Get last page number from response header of page 1
  3. Fetch and render remaining comments from page 2 to last page

@deepthivenkat
Copy link
Member Author

r? @miketaylr

@@ -371,7 +371,8 @@ issues.MainView = Backbone.View.extend({
$(document.body).addClass('language-html');
var issueNum = {number: issueNumber};
this.issue = new issues.Issue(issueNum);
this.comments = new issues.CommentsCollection([]);
this.commentPage = {page: 1};
this.comments = new issues.CommentsCollection([this.commentPage]);

This comment was marked as abuse.

@miketaylr
Copy link
Member

This looks good!

I think we can simplify some things further (and rename some stuff so it's more clear, e.g. param -> pageNumber.

Here's a diff of how we can make this a bit easier to read/understand: https://gist.github.com/miketaylr/01b9f090dda8c6e928d40c7eb08081c5

@miketaylr
Copy link
Member

(Also, I don't see the double download issue anymore -- cool that you fixed it!)

screen shot 2016-07-11 at 10 42 01 am

@deepthivenkat
Copy link
Member Author

@miketaylr I have added the review changes

@miketaylr
Copy link
Member

@deepthivenkat can you please change the commit message to match conventions (or rebase/fixup it into the previous commit)? (there's also a typo "reiew comments")

@deepthivenkat
Copy link
Member Author

Done @miketaylr

@miketaylr
Copy link
Member

Looks great, thanks!

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.

2 participants