Skip to content
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

build: minify docs HTML output #1414

Closed
wants to merge 2 commits into from
Closed

build: minify docs HTML output #1414

wants to merge 2 commits into from

Conversation

dancormier
Copy link
Contributor

This PR does what it says on the tin. Just something I whipped up in the process of dealing with another docs issue and figured I'd capture.

I think this is pretty safe to merge, but it's not urgent and we're probably best off running through the docs to make sure this change didn't cause any unexpected issues.

@dancormier dancormier added the docs Issues with the documentation site label Jul 12, 2023
@netlify
Copy link

netlify bot commented Jul 12, 2023

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 0d8a2e4
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/64af17f21bbe1700085b4780
😎 Deploy Preview https://deploy-preview-1414--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

Everything run fine in my local.

We could merge but in general I am a bit afraid of premature optimizations like this one.
We introduced a dependency and some code: do we get enough value out of it to justify those costs?

I will leave it to you to make a judgment call, I am ok to have this merged after the comment about the variable naming has been addressed. 🙂

docs/.eleventy.js Show resolved Hide resolved
docs/.eleventy.js Show resolved Hide resolved
@dancormier
Copy link
Contributor Author

Everything run fine in my local.

We could merge but in general I am a bit afraid of premature optimizations like this one. We introduced a dependency and some code: do we get enough value out of it to justify those costs?

IMO, the costs are low but so are the benefits. I didn't fully expect to merge this and it's probably not worth too much more consideration, so I'm happy to see this closed and have this PR as a little historical record if we ever change our minds.

I will leave it to you to make a judgment call, I am ok to have this merged after the comment about the variable naming has been addressed. 🙂

Closing w/o merging! Thanks for checking it out

@dancormier dancormier closed this Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issues with the documentation site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants