[WIP][Paying for College] Remove jQuery from Disclosures app #7938
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
||
| return new Promise(function (resolve, reject) { | ||
| // Completed xhr | ||
| xhr.onreadystatechange = function () { |
There was a problem hiding this comment.
You could consider using fetch instead of xhr, which may simplify the request code. See https://github.com/cfpb/consumerfinance.gov/pull/7653/files for example.
There was a problem hiding this comment.
Thanks for the tip, now that I'm closer to done with this, I'll check out fetch!
This was referenced Oct 10, 2023
mistergone
force-pushed
the
disclosures
branch
from
2ebc683 to
ccb52de
Compare
- Add cyrpress tests to ensure there are no breaking changes - Add dollar-sign.js to cover jQuery functions - Remove jQuery functions where plain JS easily addresses issues
mistergone
force-pushed
the
disclosures
branch
from
8840048 to
c5826f5
Compare
wpears
reviewed
Nov 6, 2023
* Move disclosures to fetch-logic * set school_primary_alias once
Yep! I figured that out last week - it was because my |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will remove jQuery from the P4C "Dynamic Disclosures" app.
Originally, the intent was to add a bunch of cypress tests to verify the app's functionality, then remove all the jQuery, but after a long time trying to figure out why GHA failed all the cypress tests that passed locally, I finally found that it was the jQuery itself that was causing the GHA cypress failures, so we couldn't really write a PR with passing tests while the jQuery was in there.
Thus, this PR adds the cypress tests while also removing the jQuery. Not ideal, but it is easy enough to first verify the cypress tests pass with the existing code, then check again that they pass with this branch. See "How to test this PR" for a quick command to copy/paste to test the cypress in our
mainbranch locally.Additions
dollar-sign.jswhich is a vanilla replacement for some jQuery functionsRemovals
Changes
How to test this PR
disclosuresbranchmainbranch, you can checkout just the cypress tests:git checkout disclosures -- test/cypress/fixtures/paying-for-college/ test/cypress/integration/paying-for-college/disclosures/paying-for-college/disclosuresin your cypress interface) against themainbranch, where jQuery still reigns supreme. (Make sure your JS files are built in this branch before testing!)disclosuresbranch, rebuild your JS files, and then try the cypress tests again!mainbranch if you did thegit checkout. - switching tomainand doing agit clean -iwill get it done!Checklist