-
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
Re-introduce likes on comments #8203
Conversation
Found this API issues. Can someone help here? |
Hm yeah, the work on likes on comment is older than the work on the API. It means that:
|
Rspec only gives me the API errors, cucumber works locally on my machine. So I am on a point where I appreciate some help. |
a3558a6
to
d04946e
Compare
@tclaus the current test failing doesn't look related to the API, it's an undefined in JS, you can probably fix it I think. Once done, I will do a review. |
Thanks @Flaburgan - I am still on it, and I fixed some rspec errors locally, but there is still a schema issue left over. |
cdef7a2
to
fd1d2b5
Compare
2f27aa5
to
53ee5b9
Compare
@tclaus, could you please run stylechecks locally on your machine before pushing a commit? The bot is meant as a safety-check if you miss something, not really as a tool to figure things out. :) |
All checks have passed ❤️ |
So, thank you very much for your work @tclaus Also, I didn't test the federation part (as I only have my local test pod), but I could deploy that branch on diaspora-fr.org once you fix the CSS problem and I checked the code a bit. |
@Flaburgan The mobile interface is next. For creating a notification a starting point for creating notifications would be nice - any tips here? |
Also you have a commit about http -> https links in that branch, maybe you need to rebase on develop? |
No, that's dirt from another PR. Do you have an idea to remove these file from the commits? Do I have to reset everything and commit again? On the weekend I will work on the other topics. |
Rebase on the develop branch and squash your commits, it will be gone. |
dfcec9d
to
216ccef
Compare
60d647c
to
d8e294e
Compare
|
||
&:first-child { padding-top: 20px; } | ||
.media { |
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.
Selector should have depth of applicability no greater than 3, but was 4
So thank you @tclaus I was able to push to your branch. @denschub I am glad you like it. In the future I guess we can apply the same styling to the desktop version. I also fixed most of the pronto errors. I guess it is ready for review. Edit: Jasmine tests are green locally, both with Firefox and Chrome... I will investigate. |
OK, I reviewed now everything and fixed all the things I found myself, as @tclaus doesn't have time at the moment. I now just need somebody who reviews my commits (the ones I just added, and the ones from a year ago)
@Flaburgan you broke that comments were lazy-loaded in the stream, which the test was testing for. And you didn't regenerate fixtures locally, so jasmine was still using the old fixtures where the comments were still lazy-loaded. I also fixed that so comments are laxy-loaded in the stream again. |
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.
Approving the commits and changes not made by myself ;)
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 PR still has five Pronto Ruby violations:
- app/controllers/likes_controller.rb:31 W: Metrics/AbcSize: Assignment Branch Condition size for create is too high. [<2, 22, 3> 22.29/20]
- app/controllers/notifications_controller.rb:11 W: Metrics/AbcSize: Assignment Branch Condition size for index is too high. [<18, 51, 10> 55/20]
- app/controllers/notifications_controller.rb:11 W: Metrics/MethodLength: Method has too many lines. [29/20]
- app/controllers/notifications_controller.rb:70 W: Metrics/AbcSize: Assignment Branch Condition size for read_all is too high. [<4, 32, 4> 32.5/20]
- app/helpers/notifications_helper.rb:19 W: Metrics/AbcSize: Assignment Branch Condition size for object_link is too high. [<3, 25, 7> 26.13/20]
as well as one SCSS violation:
- app/assets/stylesheets/mobile/comments.scss:131 W: Selector should have depth of applicability no greater than 3, but was 4
If we don't want to fix them, maybe we should add disable comments for those? Although I'm also not opposed to just ignoring them, but I felt like this is worth highlighting.
Either way, I have one question, but I'm approving anyway as I don't see this as blocking.
I didn't want to add disable-comments for As these violations were already there before, they just triggered now because I moved some minor things around, but otherwise didn't really touch these methods. And I also didn't want to refactor these methods in the scope of this PR, as it's already complex enough. So I just wanted to ignore them for now in this PR. I'm not sure about the violation in the CSS, if we should change that or put a comment there or just also ignore it 🤷♂️ |
Let's ignore them, then. I'm fine with that, just wanted to raise that because I noticed. :) |
Adapt to latest development User likes Set css class for inline likes on comment Re-set participation on comment likes Co-authored-by: Thorsten Claus <ThorstenClaus@web.de>
Due to historic reasons with a comment the list of all likes was sent to the frontend. This is needed just to detect if one of the likes is current users like. So if sending just the own like, the frontend can do it's job. When the frontend is refactured in any way, post and comment like handling should be improved.
When liking a comment, the post also gets a participation, and if all likes/comments get removed again, the participation also gets removed again. The only thing still not working properly is the frontend, but that is already broken when unliking a post. So it shows an invalids state in the frontend when unliking the post/comment.
3db4c65
to
b0c196a
Compare
This is the continued work initially started by @Flaburgan with PR #7918
Current status: It works (mostly): Likes on comments, persist to database, added tests.