Closed mpourismaiel closed 4 years ago
Overall looks good. Haven't looked at the failing test yet.
Also not sure on merging in the stats as well as adding the stats option. What was the reasoning here?
Sidequestion: We should probably not enable this in our starter repo as it would mean our prebuild CSS doesn't have all features. Would this apply here as well? Document, but not enable within the exampleSite, but enable within our pulled in demo at about.okkur.org aka within the okkur.org website repo? Thoughts?
The test runs without any issues on my PC. I have no ideas as to why it fails in Github workflow. Any ideas?
Not sure exactly. Seems like the mobile nav button doesn't work. This should not be related to github actions in my view. Will see what I can find.
Okay found the problem. Hugo's stats
rightfully only keeps classes that are available in the generated HTML and reports those classes to purgecss
. The required class for showing the navbar's collapse section is added via JS when user clicks the button but since that class was not available in the initial HTML displayed to the user, the styling for it has been purged.
I thought by finding the issue I could fix it quite easily but this is a bit tricky. The only solution that comes to mind right now is to add that class (.in
) to every collapse we have (and do the same in every other scenario) and remove all of those extra classes when JS loads. But that will make the initial load look bad. Maybe inline the required JS that returns navbar and dropdowns to their initial state? Maybe remove the classes and update them using HTML props? Not sure yet.
Also I wasn't able to see the failing test because I ran the tests along the development server Hugo offers. The problem arises in production mode, which we use in Github actions.
Couldn't we add a hidden dummy html node with the needed CSS classes? That way they are used close to where we would inject them via JS, but won't clutter up the frontend. Not perfect, but might work good enough. Probably needs a comment what it does aka force CSS inclusion/purge prevention for classes used in collapse.js as well as a comment in collapse.js that CSS classes are manually added on a hidden node in the partial.
What do you think?
That can work. I'm thinking of doing something a bit dirty. We can add the extra classes we need directly inside purgecss's configuration. For now we have three extra classes that we add. By changing postcss.config.js:5
from:
return els.tags.concat(els.classes, els.ids)
to:
return els.tags.concat(els.classes, els.ids, ['active', 'show', 'in'])
We can have functionality in all of our custom JS controlled elements. I actually prefer this since it's a lot less code without cluttering the HTML itself. And messing with postcss's config is not that bad since we actually provide the code in our documentation for our users to copy and paste. We can explain why we do so in there quite easily. What do you think?
I went ahead with changing our PostCSS config file and ran into an issue with PurgeCSS removing button group's input styles, which stopped the click event on buttons from triggering their callback. Finding that out and finding a fix was a head scratcher. But at last, we have successful tests and functional demo that is 40KB lighter than before! YAY \o/
I would have argued that it working out of the box with a bit more cluttered HTML > custom config, but I see your point and happy to go with that. Needs docs in a follow on PR so.
/lgtm
What this PR does / why we need it: Add PostCSS along with PurgeCSS to
exampleSite
. These remove extra CSS from the generated website and drastically reduce page size (removing around 50KB).Which issue this PR fixes: fixes #746
Release note: