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

Refactor: reduce usage of jQuery #2646

Merged
merged 12 commits into from
Jan 29, 2025
Merged

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jan 28, 2025

Part of #2493

This PR makes it so we only use jQuery for enrollment. This allows us to stop loading jQuery on every page.

Context:

I originally set out to extract inline Javascript as separate files to be loaded in with async or defer. A lot of them would have to be defer since they depend on jQuery being loaded and available.

I tried it out and saw that the performance change was negligible. There was basically no difference.

It seemed then that a more impactful change would be to reduce the need for jQuery. A lot of what we were doing with jQuery can be done with plain Javascript that is well-supported by all modern browsers.

Using https://youmightnotneedjquery.com/ and https://tobiasahlin.com/blog/move-from-jquery-to-vanilla-javascript/#document-ready as reference and testing each change thoroughly, I was able to convert over all inline Javascript except for the ones on core/enrollment/index.html and in_person/enrollment/index.html since the code there involves making AJAX requests and is more difficult to convert over.

This change showed performance improvement, specifically with First Contentful Paint and Largest Contentful Paint times, according to Lighthouse. If we want to optimize further, another PR could extract inline Javascript out to separate files and load them with async now that they don't depend on jQuery.

Before

Screenshot from 2025-01-27 20-30-42
Screenshot from 2025-01-27 20-30-57

After

Screenshot from 2025-01-27 20-31-17

Screenshot from 2025-01-27 20-31-33

@angela-tran angela-tran self-assigned this Jan 28, 2025
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Jan 28, 2025
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

this will be used instead of jQuery's ready function
the logic for feature-detection (cookies) and for logging analytics for
links can be done without jQuery.
this preserves our bug fix from bee42cf so that clicking on the modal
backdrop does not click the radio button.
jQuery is now only needed in the enrollment index for the core app and
the in-person app.

the Javascript for making AJAX requests could be converted to plain
Javascript, but that code does a lot, so not going to refactor it as
part of this effort.
@angela-tran angela-tran force-pushed the refactor/limit-jquery-usage branch from c42017b to 180206f Compare January 28, 2025 01:16
@angela-tran angela-tran changed the title Refactor: use jQuery as little as possible Refactor: reduce usage of jQuery Jan 28, 2025
@angela-tran angela-tran marked this pull request as ready for review January 28, 2025 02:45
@angela-tran angela-tran requested a review from a team as a code owner January 28, 2025 02:45
@machikoyasuda
Copy link
Member

💯 💯 💯 💯 💯 💯 💯

@lalver1
Copy link
Member

lalver1 commented Jan 28, 2025

This is looking great, and it's so easy to follow the commit history @angela-tran, thanks!

I'm noticing one thing though. The app runs fine but in the browser console I see that in http://localhost:11369/in_person/eligibility/ and http://localhost:11369/in_person/enrollment/, the new ready JS function is not defined. For example, in in_person/eligibility/:

image

I think this happens because these templates don't extend core/base.html (and this is where ready is defined), they extend admin/agency-base.html, but admin/agency-base.html does not define ready. Maybe we need to put ready somewhere were all the templates can find it?

extract inline script as an include template that is used in both
the core base template and the in-person agency base template.
@angela-tran
Copy link
Member Author

That was a great catch @lalver1, thank you!

I fixed this in e7a2ebe. Let me know if you notice anything else to change.

@angela-tran angela-tran requested a review from lalver1 January 28, 2025 21:57
@angela-tran
Copy link
Member Author

I'm noticing one thing though. The app runs fine but in the browser console I see that in http://localhost:11369/in_person/eligibility/ and http://localhost:11369/in_person/enrollment/, the new ready JS function is not defined.

One consequence of this was that the custom validity messages were not being shown, so definitely glad you brought this up. 💯

Copy link
Member

@lalver1 lalver1 left a comment

Choose a reason for hiding this comment

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

The change in e7a2ebe looks good! I also tested the reCAPTCHA feature (I had overlooked testing it during my first review) and it works fine 👍

@machikoyasuda machikoyasuda self-requested a review January 29, 2025 01:38
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Extensively tested the Benefits application flow on Firefox, Safari and Chrome:

  • Going thru the full app with keyboard only
  • Opening, closing, navigating thru modals with keyboard only
  • Clicking in and out of modals, around modals, outside of modals
  • Filling out the agency card form and triggering various validation states
  • Spinner
  • reCAPTCHA

Really well done on this PR and all the documentation! Making a big improvement on performance and code. 5 stars.

@angela-tran angela-tran merged commit bc3b890 into main Jan 29, 2025
9 checks passed
@angela-tran angela-tran deleted the refactor/limit-jquery-usage branch January 29, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants