-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fix issues with multilanguage news #1354
Conversation
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 commented on a few minor things.
news/templates/news/list.html
Outdated
fetch(currentUrl + newsId + '/remove/', { | ||
method: 'POST', | ||
headers: { | ||
'X-CSRFToken': '{{ csrf_token }}' |
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.
Could you add a comma after 'X-CSRFToken': '{{ csrf_token }}'
?
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
news/templates/news/list.html
Outdated
var removeBtns = document.querySelectorAll("[id^='remove-btn']"); | ||
|
||
for (var i = 0; i < removeBtns.length; i++) { | ||
removeBtns[i].addEventListener('click', function(event) { | ||
event.preventDefault(); | ||
|
||
var currentUrl = window.location.href; | ||
var newsId = this.getAttribute('value'); |
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.
Only use var
if it is needed, I think const
and let
should be enough in this code.
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.
Everything is now const except the interator in the for loop which is now let
2a5ef0c
to
891d200
Compare
Description
What?
Removed two leftover print calls and changed how the singular news remove button functions. Previously the button was sending a GET request which then edited the database which should not be done. This has now been changed such that it send a POST request instead.
Why?
Improves code readability and functionality
How?
Added a javascript listener to the singular remove buttons that then sends the request and redirects back to the current page
Testing
Remember to add or update unit tests for new features and changes.
What type of test did you run?
Tried removing news in different kinds of situations (Just one news item, multiple, last news item, one from the middle etc.)
Did you test the changes in
Think of what is affected by these changes and could become broken
Translation
Programming style
Have you updated the README or other relevant documentation?
Is it Done?
Clean up your git commit history before submitting the pull request!