-
Notifications
You must be signed in to change notification settings - Fork 311
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
Modify to use HTML/CSS only and remove javascript #216
Conversation
The data was already on the page, it was just hidden by default.
0371166
to
209896d
Compare
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. |
@BryanQuigley you might need some data, you can copy the |
Wow, sorry having a PEBCAK issue here... reviewing now |
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? |
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 |
01893a1
to
9121438
Compare
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. |
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. |
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:
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 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? |
(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. 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. |
Hi, regarding the bit about multiple categories: In my intro to this PR I was referring to this older comment in the original PR #139 (comment) 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 (5372e81), copy over sample data from Specifically, I believe this line of code from the branch for #139 is where the category limitation resides |
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 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. |
So I don't remember doing this, but apparently I wrote the PR that added array support to |
Closed by #237 which includes cherry-picked commits from this PR. Thanks again @lukemckinstry! |
Implements
https://github.com/azavea/geospatial-apps-tasks/issues/306
This PR picks up progress on #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 asite.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
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.Test
_datasets/
and_organizations/
folders from https://github.com/timwis/jkan-demo/ruby ./scripts/generate_categories.rb
to generate the_categories/
folder.bundle exec jekyll build
http://localhost:4000/jkan/
, datasets, organizations and categories should be visible