-
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
Single post view interaction refactorings #7182
Single post view interaction refactorings #7182
Conversation
a2c1345
to
9d8f31c
Compare
9d8f31c
to
0e541ab
Compare
0e541ab
to
ce1eb92
Compare
ce1eb92
to
d5b7e1e
Compare
d5b7e1e
to
8d43499
Compare
There was a small conflict with #7419: the timeago in the single post view didn't work anymore. I fixed that while rebasing the PR. |
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 didn't manage to review all changes of the PR, only a part of them. I'll continue with it in a few days.
tooltipSelector: [ | ||
".avatar.micro", | ||
"#show-all-likes", | ||
"#show-all-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.
This should be #show-all-reshares
I guess.
Also, you added these to the tooltipSelector
, however there is no "title"
attribute set to these elements, so no tooltip is actually displayed for these divs. Why adding them then? I think you should also either set the "title"
or remove these from the tooltipSelector.
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.
Thanks! There is no need for a tooltip, so I removed the lines.
8d43499
to
dc2b8d0
Compare
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 good to me, but here are some notes on the code
@@ -182,7 +182,7 @@ app.Router = Backbone.Router.extend({ | |||
}, | |||
|
|||
singlePost: function(id) { | |||
this.renderPage(function() { return new app.pages.SinglePostViewer({id: id}); }); | |||
this.renderPage(function() { return new app.pages.SinglePostViewer({id: id, el: $("#container")}); }); |
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.
Does it make any difference here? Why not leaving it as it was?
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 was pretty sure that there is a reason for this but I also had to search for a while. Without this change the timeagos in the single post view are missing and only appear after a few seconds. That's the conflict I mentioned here. I'll move this change to a new commit.
describe "#with_initial_interactions" do | ||
it "works with a user" do | ||
expect(@presenter.with_initial_interactions).to be_a Hash | ||
end |
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 these tests could be a little smarter and verify the contents of the hash. This data isn't checked anywhere now.
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.
dc2b8d0
to
06fc980
Compare
@svbergerem mind rebasing this? |
06fc980
to
a395388
Compare
@denschub Done. |
…al_interactions method
a395388
to
04735ce
Compare
Single post view interaction refactorings
Merged (after 9 months ;) ), thanks everyone ❤️ |
Fixes #6314. Related to #7168.
This loads the first 30 likes and reshares via gon and adds a 'Show all' link if there are more. Clicking on the link will fetch all likes or reshares.
Comments are still fetched via Ajax.