search

timwis / jkan

A lightweight, backend-free open data portal, powered by Jekyll
https://jkan.io
MIT License
218 stars 310 forks source link

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

Closed BryanQuigley closed 2 months ago

BryanQuigley commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

BryanQuigley commented 3 months ago

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

timwis commented 2 months ago

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 commented 2 months ago

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