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 :) |
svbergerem
changed the title
|
Please ping me when this is ready to be reviewed. |
svbergerem
force-pushed
the
load-spv-interactions-on-pageload
branch
from
9e4405f
to
780c428
Compare
svbergerem
changed the title
|
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.
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?
svbergerem
force-pushed
the
load-spv-interactions-on-pageload
branch
from
780c428
to
bf954bc
Compare
svbergerem
force-pushed
the
load-spv-interactions-on-pageload
branch
from
bf954bc
to
8bc4f38
Compare
|
FYI, I started reviewing this PR. Hopefully, will be able to finish by the end of the week. |
| @@ -145,6 +146,14 @@ | |||
| padding: 10px; | |||
| } | |||
|
|
|||
| .show-comments { | |||
There was a problem hiding this comment.
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.
You override text-align for SPV and and comment stream has text-center class by default:
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.
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.
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.
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: