-
Notifications
You must be signed in to change notification settings - Fork 73
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: CSS inlining in as_raw_html()
#557
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
=======================================
Coverage 90.66% 90.67%
=======================================
Files 46 46
Lines 5367 5381 +14
=======================================
+ Hits 4866 4879 +13
- Misses 501 502 +1 ☔ View full report in Codecov by Sentry. |
@rich-iannone can you say a bit about how this breaking backwards compatibility might affect users (e.g. in what situations and how?) |
I've looked a bit more -- if I'm understanding, is this what will be the result?:
I'm confused why |
The only reason is to keep the |
This will matter if users depend on raw HTML strings as inputs elsewhere (say, embedding a table in a page). However, if users are doing so, the CSS-inlined version of the HTML string is probably better for this use case since the table is more independent of the CSS defined for the page. Users might prefer the CSS-inlined version of the table anyhow since it's been noted to provide a better appearance. |
It seems like if the default is False, then it's a bit easier to explain to folks (e.g. "by default HTML renders with styles separate, use .as_raw_html() to customize behavior."; no need to explain differences in repr / as_raw_html() behavior). I'd nudge toward making its behavior match |
Thanks for weighing in! I’ll make that change. |
as_raw_html()
as_raw_html()
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 looks great -- thanks for helping me get up to speed on all this, and letting me weigh in on defaults
I think adding the |
This PR replaces #498.
This addition to the
as_raw_html()
method adds theinline_css=
argument. This option will inline CSS styles to the associated tags and remove the<style>...</style>
block from the HTML fragment. It'll be helpful for situations where you'd need to include an HTML table in an email message body (requires CSS inlining) or for embedding a table in a larger HTML document (could help to preserve more of the table's styling properties).Note that this PR results in a breaking change as calling.as_raw_html()
results in CSS-inlined HTML (since the default value of the new arginline_css=
isTrue
). This default matches that of the analogous functionas_raw_html()
in the R API.Having this new feature requires a new dependency:
css_inline
. But it does a great job with CSS inlining both with a complete document and also with an HTML fragment (both modes are used here since there is amake_page=
argument inas_raw_html()
).Here's an example for how this works in practice:
Here is the table HTML without the CSS-inlining:
With
inline_css=True
, we get the styles inlined into thestyle
attributes of the appropriate tags:Notice that the CSS-inliner does not remove existing style rules from tags. Rather styles from the
<style>
block are prepended to the existing rules.Fixes: #497
Closes: #498