Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Update Asset Pipeline Configuration #84

Merged
merged 7 commits into from
Apr 20, 2020
Merged

Update Asset Pipeline Configuration #84

merged 7 commits into from
Apr 20, 2020

Conversation

mattt
Copy link
Contributor

@mattt mattt commented Apr 18, 2020

After merging #49, I noticed a few things about the current setup:

  • The -u cssnano option in the build command was overriding / wiping out everything in the configuration file. I noticed this after generating HTML locally (downloading CSS from the raw GitHub source on master). Seeing & in the generated, minified CSS, I thought it had something to do with the Sass syntax and committed 7705c4f. However, as you mentioned, that functionality is already provided by postcss-preset-env.
  • The distinction between build and watch seemed unnecessary. If we only use watch, we don't need an additional, separate step at the end of the process.
  • There doesn't seem to be a good reason to keep unminified CSS in Resources.

This PR resolves these by removing the build script, removing and ignoring unminified CSS from Resources, and incorporating cssnano as a plugin.

@mattt mattt requested a review from kaishin April 18, 2020 14:08
Copy link
Contributor

@kaishin kaishin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind keeping the un-minified output was keeping an eye on potential regressions related to the PostCSS pipeline. But it should be fairly easy to overcome by commenting out the cssnano line in the config. 👍

@mattt
Copy link
Contributor Author

mattt commented Apr 18, 2020

The idea behind keeping the un-minified output was keeping an eye on potential regressions related to the PostCSS pipeline. But it should be fairly easy to overcome by commenting out the cssnano line in the config. 👍

That's a valid concern, and one that I want to be responsive to.

What ultimately matters is that the CSS looks good with the generated HTML, and that's a subjective measure that must be done manually. I can imagine how seeing a diff of the generated CSS would be desirable, but I see a few downsides to keeping Resources/all.css around:

  • It creates some confusion about which version is authoritative — the minified or unminified version.
  • Small changes in configuration could produce chaotic, albeit unsubstantive, changes in the resulting CSS
  • It's another blob for Git to keep track of, increasing the overall size of our repository

Here's one way to get the benefits of diffing the compiled output without the aforementioned costs:

  • Install a CSS formatting tool like js-beautify
  • Add the following to .gitattributes:
+ *.min.css diff=mincss
  • Add the following to .gitconfig:
[diff "mincss"]
	textconv = js-beautify --css
	cachetextconv = true

This would expand the CSS into a normalized, human-readable form before performing a standard diff over the text.

Not saying that we should add this immediately, but rather to share an alternative solution if later we start to feel the loss of Resources/all.css.

@kaishin
Copy link
Contributor

kaishin commented Apr 18, 2020

Neat idea! Haven't thought about using git for this sort of thing, which is a natural place to look in hindsight.

@mattt mattt merged commit eb242cf into master Apr 20, 2020
@mattt mattt deleted the cssnano-plugin branch April 20, 2020 13:22
@mattt
Copy link
Contributor Author

mattt commented Apr 20, 2020

@kaishin It's a neat technique. I learned about it as a potential solution for that perennial issue of Xcode project file merge conflicts.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants