-
Notifications
You must be signed in to change notification settings - Fork 846
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
Refactoring and removing jQuery from the vote function #1113
Conversation
…s when you click the flag button multiple times
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 is an ambitious refactoring, but I see how it saves a lot of conditionals to have two specific functions rather than one function that's repeatedly switching between the two. I do see that there's significant duplication and agree with your strategy of cleaning it up once things have settled down.
Mostly the requested changes are small style fixes, but there's a couple places to improve naming and using structure to communicate (ids vs classes).
|
||
Object.keys(reasons).map(function(k, v) { | ||
let a = document.createElement('a') | ||
a.textContent = reasons[k] | ||
a.setAttribute('data', k) | ||
a.setAttribute('href', '#') | ||
if (k === ''){ |
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.
missing space between )
and {
At some point I am going to break down and admit the js needs a linter, but not today.
The vote function was used for both stories & comments to process both voting them up & flagging them. This allowed for a lot of bugs and required onClick event functions to be created within the function, instead of as an onPageLoad event.
This pull request is step 1 of a 2 step process to split the voting into separate functions for stories or comments. It will also reduce the chance of errors by reducing the number of params passed. Instead the functions will internally know if they are handling a story or comment, rather than searching the DOM to determine what type of element it is handling .
This will double some of the code but remove complexity and chance for errors. I can later reduce some of the duplication in the follow up pull request.
This pull request includes:
Functions that have removed jQuery:
Other fixes: