Skip to content
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

Merged
merged 13 commits into from
Jul 27, 2022

Conversation

ymeynot45
Copy link
Contributor

@ymeynot45 ymeynot45 commented Jul 20, 2022

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:

  • auto adding tags when people post from sites like youtube (Still needs work due to select2)
  • flagging stories & comments

Other fixes:

  • removing html script tags and changing them to an onPageLoad function
  • prevented select2 from running before the fetch request resolved
  • a typo in the css
  • Split voting and flagging into comments & stories rather than one really large function that does all four.
    • This removes the 'story'/'comment' param from the function and all of the gates to determine which of the two it is trying to process. This makes it much easier to fix future bugs.
    • This will fix future bugs related to flagging and an event trigger that currently get created when you click the flag button.
  • Created a function to handle all of the flagging & modal removal.

Copy link
Member

@pushcx pushcx left a 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).

app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved
app/assets/javascripts/application.js.erb Outdated Show resolved Hide resolved

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 === ''){
Copy link
Member

@pushcx pushcx Jul 27, 2022

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.

@pushcx pushcx merged commit 73aa8cc into lobsters:master Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants