Modify to use HTML/CSS only and remove javascript

lukemckinstry commented 1 year ago

Implements

https://github.com/azavea/geospatial-apps-tasks/issues/306

This PR picks up progress on https://github.com/timwis/jkan/pull/139 and adds several commits.

Description

Does most work needed to move JKAN to strictly use html and css without javascript. Makes navigation and the pages for datasets, organizations, and categories available with urls and html instead of JS.

Category information (badges showing count per category) used to generated on the fly with JS. The liquid template language used in jekyll is not expressive enough to replace that, so instead I added ./script/generate_categories.rb to generate the _categories/ folder, which supports a site.categories object used to show category info in the UI, a method lifted from the MySociety data portal: https://github.com/mysociety/data_portal/blob/main/script/generate_categories.rb This script should be run whenever a jkan site is built during CI.

Notes

This PR turns off the loading of javascript in footer.html. The javascript files in ./scripts/src can be removed eventually as can some html and css intended to be used with the JS codebase.
This PR removed some UI elements like search and "show more" toggles, since those depended on JS.
https://github.com/timwis/jkan/pull/139 made the limiting assumption that category was a string (ie. limit 1 category per dataset), but OpenDataPhilly allows arrays (ie. multiple categories per dataset), this PR makes modifications to handle strings or arrays.

Test

You will need sample data in your local repo. You can copy the _datasets/ and _organizations/ folders from https://github.com/timwis/jkan-demo/
Run ruby ./scripts/generate_categories.rb to generate the _categories/ folder.
Start the server with bundle exec jekyll build
Load the site at http://localhost:4000/jkan/, datasets, organizations and categories should be visible
Since JS is no longer in use, the appearance should be the same if you disable javascript in the browser

BryanQuigley commented 1 year ago

How did you test it without javascript? If I just delete the scripts/dist/bundle.js file none of the datasets/sample department/etc work.

lukemckinstry commented 1 year ago

@BryanQuigley you might need some data, you can copy the _datasets and _organizations folders from https://github.com/timwis/jkan-demo into the corresponding folders in your local jkan repo. The scripts/dist/bundle.js file should already not be loading (see _includes/footer.html for where the code to load it is commented out)

BryanQuigley commented 1 year ago

Wow, sorry having a PEBCAK issue here... reviewing now

BryanQuigley commented 1 year ago

I misunderstood that categories would break (I thought it was just going to be an ordering thing), but they still get counted.

I think we likely are going to want to hold off until we can add the category piece from mysociety.

@timwis this would get us most of the way to using no javascript, mostly just leaving search as the last big user. Thoughts?

lukemckinstry commented 1 year ago

following discussion with @BryanQuigley updating this PR to include support for categories. Latest 2 commits have it almost there but this is still WIP, need to finish some navigation details. Planning to finish next week at which point I'll update the test instructions. The key addition will be to run ruby ./scripts/generate_categories.rb to build the _categories directory.

lukemckinstry commented 1 year ago

added commits here to finish the work described in the previous comment to fully move to html-only functionality without JS. Updated the PR description and testing notes to reflect these latest updates.

timwis commented 1 year ago

Hey Luke! 👋🏻 Sorry for the delay, been doing Christmas stuff with the family. I have some down time this week, though, and will dive in to this.

timwis commented 1 year ago

Okay, I've dove into this and #139 for a few hours this morning, and I think I've got my head wrapped around the proposed changes.

First of all, thanks for your work on this! I feel terrible that I never got @dracos' #139 merged, and you're right to bring the need for progressive enhancement back to the table.

From what I've gathered, this PR does the following:

Merges #139 and fixes an issue with how it groups categories
Disables (nearly) all JavaScript functionality entirely
Generates category pages via a build-time script (supporting filtering by category without JS)

Regarding (1), I've just tested #139, and can't seem to reproduce the category array/string bug mentioned there. Can you help me understand how to reproduce the issue? I'm wondering if maybe it's been fixed by a newer version of Jekyll (I've just got the fork up to date). The only issue I see with it is that it only shows the first 15 organisations.

Regarding (2), can you help me understand why we'd want to disable/remove JavaScript entirely? I would think we'd want to instead use progressive enhancement, making sure all the site's features work without JavaScript, but then sprinkling in JavaScript to enhance things where appropriate (e.g. those 'see more' buttons). (Also note that #213 will significantly reduce the amount of JavaScript JKAN uses)

Regarding (3), the only thing is that JKAN currently runs on GitHub Pages (building and hosting), which doesn't allow us to introduce any additional build steps. We are discussing potentially changing the default hosting provider to Netlify in #204, but that's a pretty significant change. Some alternatives could be:

a. Use GitHub Actions to build and deploy to GitHub pages, per #199 b. Use a plugin like jekyll-datapage_gen to generate the pages (though this still requires building it outside of GitHub Pages) c. Try adding the feature to the Jekyll codebase, per jekyll/jekyll#5207 (long shot, but adding for completeness!) d. Replace _data/categories.yml with a _categories collection, and just use Jekyll's built-in collection generator.

I think (d) may be the best way forward, considering it would require no extra build step, and it would help with the problem described in #207 anyway. The only downside is that it's a breaking change (but possibly one that we could mitigate with backwards-compatibility).

With that in mind (and pending your thoughts on the removing-JS question), can I suggest that we merge #139 (I've just got it up to date with the latest gh-pages), spin off a separate PR to fix the category string/array bug if it still exists, and focus this PR on the best way to generate category pages?

BryanQuigley commented 1 year ago

(2) - It wasn't in the original goal, but if we are restricting to use javascript only for progressive enhancement we might as well drop everything and then add javascript back as needed.

(3) (a) - I hadn't realized that switching to the more powerful Github actions would be required for this change. But that seems reasonable enough.
(d) I didn't consider that an option when considering this, but I guess it is now. I think now is the time to make breaking changes.

Re: 139 - there are still conflicts with the merge. I haven't looked at what we might lose.

I forgot, and I'll let Luke fill in the bigger details, but we need to generate categories in order for it to really work at all with JS. The multiple category bit is what necessitated building more of the static generation pieces.

timwis commented 1 year ago

Okay sure. If it helps, I got #139 up-to-date with 5b4bf87. But do let me know what issues you ran into with the multiple category bit; I couldn't reproduce the issue when I tried.

lukemckinstry commented 1 year ago

Hi, regarding the bit about multiple categories: In my intro to this PR I was referring to this older comment in the original PR https://github.com/timwis/jkan/pull/139#issuecomment-381349467 which was pointing out that following the work done up to that point to make the datasets page display as html-only, the categories menu was not displaying correctly or the same as it had been in the js-based version of the page. It seems like the PR writer thought tallying and displaying the categories stats using the liquid template language would work (whether each dataset had one category or multiple), but I tried pickup up and fixing that code and was unable to make it work. What I ended up doing in this version of the PR was following the method used by the mysociety data portal (https://github.com/mysociety/data_portal/blob/main/script/generate_categories.rb) to generate the category files in a script to be run whenever the site is built.

If you would like to reproduce the original issue from PR #139, you can go back to the last commit before we picked up work (https://github.com/timwis/jkan/pull/139/commits/5372e814b019701553810361b03fedc0f2a9bbe5), copy over sample data from _datasets in https://github.com/timwis/jkan-demo/, then run the site.... at that point the categories menu on the datasets page will display differently when JS is enabled or disabled.

Specifically, I believe this line of code from the branch for #139 is where the category limitation resides {% assign c = site.datasets | where:"category", category | size %} It is iterating through each category and trying to count the number of datasets where this matches the value in the dataset.category field. I tried simply updating this to a statement like "equals" the string or the array "includes" but the liquid template language is not expressive enough. This limitation led me to suggest and implement the script method that mysociety used.

lukemckinstry commented 1 year ago

Looking at the update you made to #139, it looks like it does fix the categories display! As I commented on the PR, it does look like the organizations section is not working the same between JS and No-JS, perhaps the same fix could be added there?

I'm curious how the fix works. In the filter-datasets.html file the line I singled out in the previous comment as not working (since our data has a mix of string and array categories) seems in fact to be working, but from what I can tell from the jekyll docs nothing changed from before and it should not work.

If we get the organizations section sorted out as well it could make sense to merge #139 and go from there. But one advantage of generating the categories page by script (or jekyll built in collection generator as you suggested) is then we can allow the user to view all datasets for a specific category, which this setup doesn't include.

timwis commented 1 year ago

I'm curious how the fix works.

So I don't remember doing this, but apparently I wrote the PR that added array support to where 😆 It looks like my tests included the case of mixing strings and arrays. Perhaps #139 was using an older version of Jekyll until my recent commit? Looking at the Gemfile.lock of its various commits, I think that may be the case.

timwis commented 1 year ago

Closed by #237 which includes cherry-picked commits from this PR. Thanks again @lukemckinstry!

timwis / jkan