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

feat: Minify html,css,js #854

Merged
merged 6 commits into from
May 24, 2021
Merged

feat: Minify html,css,js #854

merged 6 commits into from
May 24, 2021

Conversation

tglman
Copy link
Contributor

@tglman tglman commented May 5, 2021

  • Minifies rendered documents
  • Minifies js, and css assets

Minification is disabled by default, for backward and text compatibility. This can be changed on an extension-by-extension basis.

This doesn't touch the unused config crate.

@epage
Copy link
Member

epage commented May 6, 2021

Thanks for doing this!

Looks like something needs to be resolved with the tests.

I see there are three commits. Were these written for being preserved or would you want me to squash the PR?

As for the committed failure, consider that more of a warning than an error. I can help with resolving it, resolve it at merge, or we can leave it.

Also i noticed that there is a nested crate that duplicate some configurations, is it needed to update that crate as well ?

... I guess I got interrupted in the middle of that work. Don't worry about it. I am taking a break from cobalt to wrap up another project so I can focus on it but after that, I'm working on a re-architecting of cobalt to make it easier to add features.

@epage epage changed the title Add minification support for html,css,js feat: Minify html,css,js May 6, 2021
src/document.rs Outdated

#[cfg(feature = "html-minifier")]
fn minify_if_enabled(html: String, context: &RenderContex) -> Result<String> {
if context.minify.html {
Copy link
Member

@epage epage May 6, 2021

Choose a reason for hiding this comment

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

We might render other types of data than html

Ideas:

  • Limit this it .html extension
  • Switch control for this over to the frontmatter. We'd then move the other minification options to be under assets in the config.
  • Leave it as-is for now, see how it works out, and explore changing it later

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, yes i will try to add a double check that the final asset is actually an html file.

src/cobalt_model/assets.rs Outdated Show resolved Hide resolved
@epage
Copy link
Member

epage commented May 6, 2021

Sorry I didn't notice these two questions before; it was when writing up a description for this that I double checked how things worked and had these questions.

@tglman
Copy link
Contributor Author

tglman commented May 8, 2021

Hi,

I fixed the issue i had in the tests, but the run keep failing on something that do not seems related to my changes: https://dev.azure.com/cobalt-org/cobalt-org/_build/results?buildId=749&view=logs&j=50044035-f94f-5642-804a-a0db99902e68&t=ab898a7a-9071-5c69-f447-a57931fa134d

For the commit, when I completed the rest i think is OK to squash them in a single commit

@epage
Copy link
Member

epage commented May 10, 2021

The failure looks like an issue in syntect. I've opened trishume/syntect#334

@epage epage merged commit 657abc9 into cobalt-org:master May 24, 2021
@epage
Copy link
Member

epage commented May 24, 2021

Thanks for the work on this! Do you need me to cut a release for you?

@tglman
Copy link
Contributor Author

tglman commented May 24, 2021

Hi,
No problem, don't need a release, feel free to do it when you feel is appropriate.

Have a good day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants