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

Closed
wants to merge 1 commit into 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?

@timwis
Copy link
Owner

timwis commented Jun 2, 2024

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

Sorry for the delay! index.js imports bootstrap.native, a JS library that reimplements the interactive functionality of bootstrap in vanilla JS (otherwise it would be jQuery). We only pull in the 'collapse' feature, which we use in the header on every page, to enable the navbar to expand/collapse on small screens. It doesn't look like that feature is working on OpenDataPhilly.org (try on mobile or narrow browser window), but it should work normally.

@BryanQuigley
Copy link
Collaborator Author

Thanks! Wow - not sure how I missed that it was broken. May revisit this, but for now let's close it out.

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.

2 participants