-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Load initial comments in the SPV on pageload #7168
base: develop
Are you sure you want to change the base?
Load initial comments in the SPV on pageload #7168
Conversation
I'm not sure about loading the interactions on page load in the SPV. Because on posts with many likes/comments it would really slow down the initial page load. For example this post: https://pod.geraspora.de/posts/571518 But I like the other two commits :) |
Please ping me when this is ready to be reviewed. |
9e4405f
to
780c428
Compare
This PR has been updated and is now ready to review. It loads the last 10 comments in the single post view on pageload and shows the “show more comments” link if there are more than 10 comments available. If a comment should be highlighted that's not available on pageload, it loads all comments and then highlights the comment. |
lib/diaspora/commentable.rb
Outdated
return [] if comments_count == 0 | ||
# DO NOT USE .last(3) HERE. IT WILL FETCH ALL COMMENTS AND RETURN THE LAST THREE | ||
# INSTEAD OF DOING THE FOLLOWING, AS EXPECTED (THX AR): | ||
comments.order("created_at DESC").limit(count).includes(author: :profile).reverse |
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.
Looks to me like this has been fixed, so we could use
comments.order("created_at").last(count).includes(author: profile)
instead. Can someone confirm?
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.
Yes, that's true 👍
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. Thank you. :)
780c428
to
bf954bc
Compare
bf954bc
to
8bc4f38
Compare
FYI, I started reviewing this PR. Hopefully, will be able to finish by the end of the week. |
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 found no serious issues with this code. There are some little things and questions, but otherwise looks good to me.
@@ -145,6 +146,14 @@ | |||
padding: 10px; | |||
} | |||
|
|||
.show-comments { |
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.
Currently this style is different from how it looks in the stream_post_views.js
. Do we have any reason for it to differ in this two places? May that be better that the "Show more" and the loader look the same way both in the stream and in the SPV?
margin-top: -$line-height-computed - 11px; | ||
text-align: center; |
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.
You override text-align
for SPV and and comment stream has text-center
class by default:
<div class="loading-comments comment text-center hidden"> |
So this line is no-op here.
@@ -9,10 +9,91 @@ describe("app.views.SinglePostCommentStream", function() { | |||
expect(this.view.CommentView).toBe(app.views.ExpandedComment); | |||
}); | |||
|
|||
it("calls highlightPermalinkComment on hashchange", function() { |
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.
nit: this name is misleading. initialize
doesn't call highlightPermalinkComment
, it sets a handler for hashchange
event.
expect(this.view.$(lastGuidSelector)).not.toHaveClass("highlighted"); | ||
expect(this.view.$(firstGuidSelector)).toHaveClass("highlighted"); | ||
}); | ||
}); |
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.
Nit: Maybe worth adding a test for scrolling as well?
it("shows the new comment form wrapper", function() { | ||
this.view.render(); | ||
this.view.$(".new-comment-form-wrapper").addClass("hidden"); | ||
expect(this.view.$(".new-comment-form-wrapper")).toHaveClass("hidden"); |
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.
Do we really need to verify the effect of addClass
here?
Fixes #4816, #5611 and #6594.
The first commit loads all post interactions (including comments) on page load in the SPV.
The second commit fixes the format for post interactions in the stream.
The last commit adds a spinner when loading comments in the stream: