mrml icon indicating copy to clipboard operation
mrml copied to clipboard

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

Open sciyoshi opened this issue 8 months ago • 7 comments

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 avatar Apr 24 '25 04:04 sciyoshi

Codecov Report

:x: Patch coverage is 94.05941% with 6 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 92.83%. Comparing base (5311e58) to head (25350b2). :warning: Report is 156 commits behind head on main.

Files with missing lines Patch % Lines
packages/mrml-core/src/prelude/parser/output.rs 0.00% 4 Missing :warning:
packages/mrml-core/src/mjml/render.rs 96.15% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   92.89%   92.83%   -0.06%     
==========================================
  Files         227      206      -21     
  Lines       12203    11353     -850     
==========================================
- Hits        11336    10540     -796     
+ Misses        867      813      -54     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar May 12 '25 11:05 codecov[bot]

@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

jdrouet avatar May 12 '25 13:05 jdrouet

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 avatar May 12 '25 13:05 sciyoshi

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.

sciyoshi avatar May 12 '25 13:05 sciyoshi

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 :wink:

jdrouet avatar May 12 '25 13:05 jdrouet

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.

jdrouet avatar May 20 '25 15:05 jdrouet

@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 avatar May 21 '25 04:05 sciyoshi

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

kivra-mikgra avatar Jul 01 '25 13:07 kivra-mikgra

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)

Stranger6667 avatar Jul 01 '25 15:07 Stranger6667

👋 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 🤞.

QuentinFchx avatar Dec 05 '25 13:12 QuentinFchx

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

jdrouet avatar Dec 05 '25 14:12 jdrouet

Done - also bumped css-inline to 0.18.0.

sciyoshi avatar Dec 05 '25 23:12 sciyoshi