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

Remove jquery #241

Draft
wants to merge 2 commits into
base: v2
Choose a base branch
from
Draft

Remove jquery #241

wants to merge 2 commits into from

Conversation

timwis
Copy link
Owner

@timwis timwis commented Feb 14, 2023

Nearly finished, but not quite — just pushing this up as a draft for visibility.

Still need to:

  • Get components/dataset-display.js working
  • Pull in the minimal JS needed for bootstrap components like the nav bar

Note that it will be worth comparing bundle sizes before merging, to verify whether this PR adds value (tree shaking has come a long way)

@timwis timwis changed the base branch from gh-pages to parcel February 14, 2023 08:14
Base automatically changed from parcel to v2 February 14, 2023 18:15
Note that we must use a tilde in package.json to prevent it upgrading to v4.2.0, which dropped support for bootstrap 4
@timwis
Copy link
Owner Author

timwis commented Feb 15, 2023

Bundle size comparison

  • Current (with jquery): 170.11 KB
  • With jQuery and default bootstrap js removed: 106.43 KB

We could shave off another large chunk (probably 60 KB) by getting tree shaking working with lodash, but it's not immediately possible because of the use of the library's chain method (which pulls in the whole library, as I understand it), so would require a refactor in two of the components.

Last thing I want to do on this branch is just check the browser support to make sure we're comfortable with the changes.

@timwis timwis marked this pull request as ready for review February 15, 2023 08:12
@timwis timwis marked this pull request as draft February 15, 2023 08:13
@BryanQuigley
Copy link
Collaborator

Was looking to review this, but it looks like bootstrap 5 dropped JQuery anyway. Does this mean we would just stick on bootstrap instead of bootstrap.native?

@timwis
Copy link
Owner Author

timwis commented Mar 20, 2023

Yep, that's right. To be honest, I got to the end of this pull request and just felt like it made readability worse without much benefit. Happy to carry on with it once we've got bootstrap 5 ready, though I'm half-tempted to just rewrite it (perhaps in stimulus, which takes a similar multi-page app approach) since it's so little JS at this point.

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.

None yet

2 participants