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

JavaScript cleanup #421

Merged
merged 28 commits into from
Jun 3, 2020
Merged

JavaScript cleanup #421

merged 28 commits into from
Jun 3, 2020

Conversation

eddieantonio
Copy link
Member

@eddieantonio eddieantonio commented Jun 3, 2020

This is a series of refactors, style tweaks, and changes, including:

  • phase out use of jQuery: not 100% complete
  • apply ESLint on all of our regular files
  • use fetch() consistently instead of XMLHTTPRequest

I mostly worked on index.js, not really doing anything more than running ESLint on recordings.js.

EDIT: one casualty is waiting for the fetch() in our loading bar tests because Cypress doesn't currently support fetch() BUT, they're working on it!

@eddieantonio eddieantonio added Improvement Expansion or improvement of a current functionality that does already work and meets previous specs javascript Pull requests that update Javascript code labels Jun 3, 2020
@cypress
Copy link

cypress bot commented Jun 3, 2020



Test summary

65 0 6 0


Run details

Project cree-intelligent-dictionary
Status Passed
Commit 3ce98b3 ℹ️
Started Jun 3, 2020 4:42 PM
Ended Jun 3, 2020 4:45 PM
Duration 02:14 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 73

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@Madoshakalaka
Copy link
Collaborator

@eddieantonio why shouldn't we use jquery

@eddieantonio
Copy link
Member Author

@eddieantonio why shouldn't we use jquery

It's one more dependency that we don't really need when we're doing DOM manipulation that can be done without jQuery. Do the same with a lot less code!

}

/**
* use xhttp to load search results in place
* use AJAX to load search results in place
Copy link
Collaborator

@Madoshakalaka Madoshakalaka Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eddieantonio why is ajax favoured over xhttp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AJAX refers to any asynchronous HTTP request, whether it's done with the crusty old XMLHttpRequest API or the newer, less error-prone fetch() API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah was asking why use fetch over XMLHttpRequest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah was asking why use fetch over XMLHttpRequest

I've personally always went with XMLHttpRequest because it's what I was taught/learned first! Fetch is new to me but I'm aware that it is the future so I'll try to work at using it more go forward.

If I remember right, fetch handles some things better than XMLHttpRequest does (did?). I think I have a short tutorial tucked away for why the newer is better: just holler and I can pull it over the weekend or something 💪

Copy link
Collaborator

@Madoshakalaka Madoshakalaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Glad to know the javascript is working without me

@eddieantonio eddieantonio merged commit d7c5951 into master Jun 3, 2020
@eddieantonio eddieantonio deleted the js-cleanup branch June 3, 2020 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Expansion or improvement of a current functionality that does already work and meets previous specs javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants