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

Drop javascript from footer and add to only the pages it's needed on #285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BryanQuigley
Copy link
Collaborator

The goal is to make it easier to restructure/reduce javascript in the future if we only need to test the 3 pages it acts on.

The goal is to make it easier to restructure/reduce javascript in the future
if we only need to test the 3 pages it acts on.
@timwis
Copy link
Owner

timwis commented Apr 22, 2024

Hm, it looks like the javascript components are also used by /layouts/_dataset.html and the navbar toggle (and any other bootstrap-driven JS like tooltips) would be on every page.

But, stepping back a bit, can you help me understand the rationale behind this change? If I understand correctly, it really just removes the JavaScript from the homepage (which would also break the navbar toggle).

If it's a performance or bundle size concern, perhaps we'd be better off replacing jquery and lodash with vanilla JS?

@BryanQuigley
Copy link
Collaborator Author

My goal was actually just to make it easier to identify the places that use javascript so if we want to change off of jquery/lodash in the future it's easier/more obvious where we need to test.

I'm totally missing the bootstrap js or navbar differences from my build (or my ability to see it). Will revisit after I take a closer look.

@timwis
Copy link
Owner

timwis commented Apr 23, 2024

Ah, okay. In that case, I wonder if this search would make it even easier to see what to test?

@BryanQuigley
Copy link
Collaborator Author

Yes, but I still don't see the navigation javascript code being actually used. What does it do?

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