Skip to content

feat: add CSS inlining support for mj-style with inline attribute#537

Merged
jdrouet merged 11 commits intojdrouet:mainfrom
sciyoshi:fix-css-inlining-issue-17
Dec 12, 2025
Merged

feat: add CSS inlining support for mj-style with inline attribute#537
jdrouet merged 11 commits intojdrouet:mainfrom
sciyoshi:fix-css-inlining-issue-17

Conversation

@sciyoshi
Copy link
Copy Markdown
Contributor

Description

This PR implements CSS inlining support for MJML templates with the 'inline' attribute on mj-style elements.

Changes

  • Added support for the 'inline' attribute on mj-style elements
  • Implemented CSS inlining using the css-inline crate (version 0.14.4)
  • Modified the rendering process to differentiate between regular styles and inline styles
  • Updated the header mechanism to store inline styles separately
  • Added error handling for CSS inlining failures
  • Added test cases to verify the CSS inlining functionality

How It Works

  • When an mj-style element has the 'inline' attribute, its content is stored in the header's inline_styles collection
  • During the final rendering process, the CSS inliner is applied to the HTML output only if there are inline styles
  • Non-inlined styles are still preserved as regular style tags

Fixes #17

@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch 3 times, most recently from 0ad8d6d to c824b71 Compare April 24, 2025 04:16
Copy link
Copy Markdown
Owner

@jdrouet jdrouet left a comment

Choose a reason for hiding this comment

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

Several questions:

  • As this been generated by an AI?
  • Did you run a benchmark?

Moving forward:

  • Could you put this behind a feature flag?

@codecov
Copy link
Copy Markdown

codecov bot commented May 12, 2025

Codecov Report

❌ Patch coverage is 94.44444% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.84%. Comparing base (5311e58) to head (ce90fe1).
⚠️ Report is 157 commits behind head on main.

Files with missing lines Patch % Lines
packages/mrml-core/src/prelude/parser/output.rs 0.00% 4 Missing ⚠️
packages/mrml-core/src/mjml/render.rs 96.15% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   92.89%   92.84%   -0.06%     
==========================================
  Files         227      206      -21     
  Lines       12203    11357     -846     
==========================================
- Hits        11336    10544     -792     
+ Misses        867      813      -54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from c824b71 to 79ed015 Compare May 12, 2025 12:52
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from 79ed015 to 92c392f Compare May 12, 2025 13:01
@jdrouet
Copy link
Copy Markdown
Owner

jdrouet commented May 12, 2025

@sciyoshi for me to keep track of your changes before approving the workflows AND for my reviews to stay attached to your code, could you push new commits instead of push-forcing?
Thanks in advance

@sciyoshi
Copy link
Copy Markdown
Contributor Author

Thanks @jdrouet, and my apologies, will do. Just attempting to fix the latest CI issues.

As for your questions: the first pass was mostly generated with Sonnet 3.7 Thinking, but I had to make a number of manual changes to get it working. I have not run any benchmarking yet on the new code.

It is possible to add a feature flag, but the behavior won't apply unless there's any inline styles in the input, so I wasn't sure if that was necessary.

@sciyoshi
Copy link
Copy Markdown
Contributor Author

The only change from the original commit so far was to fix a clippy error and to add blocking as an option for reqwest to attempt to fix the current build failure.

@jdrouet jdrouet added the AI generated This PR uses AI label May 12, 2025
@jdrouet
Copy link
Copy Markdown
Owner

jdrouet commented May 12, 2025

The only change from the original commit so far was to fix a clippy error and to add blocking as an option for reqwest to attempt to fix the current build failure.

The means that css_inline uses reqwest inside. I think, instead, you should disable some features in css_inline 😉

Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from 1c564f9 to 834de09 Compare May 12, 2025 13:39
sciyoshi added 2 commits May 12, 2025 09:53
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from fe1fa60 to fea71ac Compare May 12, 2025 13:53
@jdrouet
Copy link
Copy Markdown
Owner

jdrouet commented May 20, 2025

Hey,
Sorry for the late reply and thanks for your changes.
Could you put the css_inline behind a feature flag disabled by default? It's not just for the performance but also for the binary size.

@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from 604a7ab to e1cef80 Compare May 21, 2025 04:14
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from e1cef80 to 085c1d0 Compare May 21, 2025 04:20
@sciyoshi
Copy link
Copy Markdown
Contributor Author

@jdrouet sure, I added a css-inline feature which is disabled by default, as well as a warning that gets emitted if you try to use <mj-style inline="inline"> when the build doesn't have the feature enabled.

@sciyoshi sciyoshi requested a review from jdrouet May 22, 2025 00:34
@kivra-mikgra
Copy link
Copy Markdown

We're really looking forward for this change let me know if I can help in any way!

@Stranger6667
Copy link
Copy Markdown

css-inline author here. If you need any adjustments from the crate side, don't hesitate to tag me :)

As performance was mentioned, I am wondering if it would be useful to have some CSS pre-compilation feature in css-inline? I.e., if you have all the CSS upfront, it could be useful to parse it just once and then apply inlining without re-parsing. Depending on the input, it could be a significant win (talking about ~20% in my benchmarks)

@QuentinFchx
Copy link
Copy Markdown

👋 Hi,
Gently bumping this PR, we are also very much interested in this feature! This is currently the only thing preventing us from migrating to mrml 🤞.

@jdrouet
Copy link
Copy Markdown
Owner

jdrouet commented Dec 5, 2025

@sciyoshi could you resolve the conflicts so that we can trigger the tests?

…sue-17

Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>

# Conflicts:
#	readme.md
#	readme.md~HEAD
@sciyoshi
Copy link
Copy Markdown
Contributor Author

sciyoshi commented Dec 5, 2025

Done - also bumped css-inline to 0.18.0.

Copy link
Copy Markdown
Owner

@jdrouet jdrouet left a comment

Choose a reason for hiding this comment

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

We are almost there, thanks for your changes!
Could you also sign your commits?

mrml = { version = "5.1.0", path = "../mrml-core", features = [
"http-loader-ureq",
"local-loader",
"css-inline",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

thought
I have mixed feelings around enabling this by default. This will impact the performance by default 🤔
Although probably not when inline CSS is not used.
I'm interested to see some benchmarks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My thought here is that it should be enabled by default for the Python lib, because otherwise there's no way to enable it for a consumer without rebuilding this package, and the performance impact if you are not using inlining should be negligible.

Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi force-pushed the fix-css-inlining-issue-17 branch from 25350b2 to 1e4dfd8 Compare December 8, 2025 15:40
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
Signed-off-by: Samuel Cormier-Iijima <samuel@cormier-iijima.com>
@sciyoshi sciyoshi requested a review from jdrouet December 8, 2025 17:35
@jdrouet jdrouet merged commit 48b592d into jdrouet:main Dec 12, 2025
19 of 20 checks passed
@github-actions github-actions bot mentioned this pull request Dec 5, 2025
@sciyoshi sciyoshi deleted the fix-css-inlining-issue-17 branch December 12, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI generated This PR uses AI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

handle mj-style with inline attribute

5 participants