-
-
Notifications
You must be signed in to change notification settings - Fork 34
feat: Add option to minify CSS #559
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
Conversation
CodSpeed Performance ReportMerging #559 will not alter performanceComparing Summary
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
==========================================
+ Coverage 89.11% 89.25% +0.14%
==========================================
Files 18 18
Lines 2048 2084 +36
==========================================
+ Hits 1825 1860 +35
- Misses 223 224 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| [dependencies.css-inline] | ||
| version = "0.17" | ||
| path = "../../../../css-inline" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is not properly documented, but it is the way to make an installable gem. There was a comment somewhere in closed issues (not sure if you mentioned that one on the original message, I’ll take a closer look when I am back to my pc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was doing my best to follow the conversation starting in #485 (comment), but I'm not sure if I did the right thing 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a more detailed explanation by Magnus' author. Briefly, the referenced path won't be packaged with the published crate (in contrast with PyO3 & related build system that packages everything needed), and during the installation time, cargo won't find it.
Right now, it is fine to have it like this. After releasing the Rust crate, I'll make the needed changes and will release the Ruby side separately
|
Outstanding work! Thank you so much :) Re: MSRV - let's bump it to 1.80 |
| ) -> Result<(), InlineError> { | ||
| writer.write_all(name.as_bytes())?; | ||
| writer.write_all(STYLE_SEPARATOR)?; | ||
| if minify_css { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perf: I'd like to acknowledge that this branch affects performance a bit. I am fine with keeping it as is, as the impact is minimal. But at some point, I'd like to have generics that will abstract this behavior and allow for a monomorphized version without branching (if it won't affect the binary size / compile time much)
|
Thanks for reviewing! Bumped MSRV to 1.80. |
|
Thank you so much! I'll check the failing builds and will proceed with a new release next weekend :) |
Related to #12 (comment)
Adds a new
minify_cssoption. When set totrue, style attributes will not contain trailing semicolons or spaces between style properties and values, e.g.Unrelated to the new option, I also made the following changes:
cargo fmt --all -- --checkon the Java bindingscargo clippy -- -D warningson the Java bindingstestRuntimeOnlyconfig to the Java gradle build to fix the error seen hereThere are some outstanding issues/questions:
keep_at_rulesoption #485 (comment) -- is this correct?rayon-core. This is also present in the latestmasterbuild, but let me know if/how I can resolve itkeepAtRulesbe added tobindings/javascript/js-binding.d.ts? See comment belowkeep_at_rulesbe added tobindings/python/css_inline.pyi? See comment below