-
Notifications
You must be signed in to change notification settings - Fork 35
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
Limit resource likes #87
base: master
Are you sure you want to change the base?
Limit resource likes #87
Conversation
…ing or incrementing like
…n't already liked
app/assets/javascripts/resources.js
Outdated
|
||
function processLike(event, target){ | ||
event.preventDefault(); | ||
resourceID = target.parentNode.parentNode.action.split('/')[4] |
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 way you rely on the absolute URL containing 5 forward slashes ('/'), which is not a good idea. That code would break if we change our URL paths. What you know (and what is independent of the rest of the path) is that the URL ends on '<resource_id>/like'. So instead of getting the 5th element of the array, I would use 'target.parentNode.parentNode.action.split('/').slice(-2)[0]'. That way you first reduce the array to the two last elements, and then choose the resource_id.
@probinson2015 I left some comments - looks good for the rest! 👍 |
Thank you @natsteinmetz. I've updated accordingly. |
Hello @natsteinmetz, I've updated the code and would like for you to do another code review, this branch is not ready to be merged. On decrement likes, I’d like to be able to use ajax and call a different action other than the like route that is currently attached to the form. I was going to manually code it out by building out the ‘unlike’ path then use jQuery to replace the form action text with the unlike path but that isn’t working how I expected it to. I am receiving a 500 Internal Server error. There are many reasons why this may not work and I’m sure there is a cleaner way to do it but I haven’t been able to find it. What I am thinking is to add another (hidden) button to the form that calls the unlike path. Can you provide guidance on how to execute this cleanly? The cookie part is working fine, so when the heart is clicked:
I am at the point where I want to get the database updated accordingly within the 'decrementLike' function. Thank you! |
@probinson2015 - this PR is very old - is the code still good/is the task still worth doing? |
Hi @natsteinmetz, new commits pushed here. It's working as expected. Can you review please? I'll write some unit tests also. Thanks! |
app/assets/javascripts/resources.js
Outdated
var form = $(target).parents('form'); | ||
resourceID = target.parentNode.parentNode.action.split('/').slice(-2)[0] | ||
if (getCookie("_shescoding_likes") === ""){ | ||
createNewCookie(target, resrouceID); |
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.
Typo, 'resrouceID' should be 'resourceID'.
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.
Ha, thank you. will update!
app/assets/javascripts/resources.js
Outdated
event.preventDefault(); | ||
var form = $(target).parents('form'); | ||
resourceID = target.parentNode.parentNode.action.split('/').slice(-2)[0] |
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 needs to be slice(-3)[0]. My mistake for suggesting slice(-2)[0] in an earlier review. I think at that point that did work, because resources without likes did end on '...//like. It would not have worked when trying to process likes of resources with previous likes.
Either way, as of the current status, each URL ends on a number behind /like, so slice(-3) should always give the correct result.
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.
Ok. Sounds good.
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 two bugs, as commented in the code - with those fixed, the behavior is as expected. This is good to go once:
- the bugs have been fixed, and
- tests have been added.
Thanks, Porsha!
Hi @natsteinmetz, new commits pushed here. It's working as expected. Can you review please? I'll write some unit tests also. Thanks!