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

CSS improvements #45

Merged
merged 1 commit into from Nov 17, 2023
Merged

Conversation

caseyjhol
Copy link
Contributor

@caseyjhol caseyjhol commented Sep 28, 2023

@caseyjhol caseyjhol changed the title Fix #44: Don't escape HTML when decoding CSS improvements Sep 28, 2023
@caseyjhol
Copy link
Contributor Author

caseyjhol commented Sep 28, 2023

@FelixSchwarz I'm trying to upgrade to css-inline v0.11.0 to take advantage of performance improvements added in v0.10.x, but it requires using the keep_link_tags and keep_style_tags arguments, which older versions of css-inline don't support. 0.8.3 is the latest version that supports Python 3.6, so we're getting "unexpected keyword argument" errors when trying to use it in the 3.6 tests. Not sure how you'd like to handle this. I think a try/except block might be our only option (besides dropping support for v3.6, which seems extreme), but maybe you have another idea.

@Stranger6667
Copy link

Btw, I’d be curious how much the update improves performance in your scenario - I have very few benchmarks so it would be cool to learn about real life effect of all that performance changes :)

@caseyjhol
Copy link
Contributor Author

@Stranger6667 Just to consider all options, how feasible would it be to add back support for Python v3.6 to css-inline?

@Stranger6667
Copy link

@caseyjhol Supporting Python 3.6 requires downgrading pyo3 to 0.15.2 which does not support Python 3.11 and the number of bugfixes between the current pyo3 version and that one is quite big. It does not feel reasonable for css-inline to downgrade that much, but I guess, generally, pyo3==0.15.2 should be compatible with the current binding's implementation. I.e. it could be a fork / different branch that could use older pyo3 and released under a different name on PyPI (e.g. css-inline-py36) and used as a conditional dependency for Python 3.6 only.

@FelixSchwarz
Copy link
Owner

0.8.3 is the latest version that supports Python 3.6, so we're getting "unexpected keyword argument" errors when trying to use it in the 3.6 tests. Not sure how you'd like to handle this.

I think we can make a point that css inlining is only support for Python >= 3.7. We are still using mjml in production with Python 3.6 - probably until RHEL 7 support ends and I can justify spending on mjml-python more easily if we can use mjml in our infrastructure.

However we don't need css inlining for the Python 3.6 deployment, so this PR seems like a good reason to raise that requirement. I would release the next version as 0.10.0 as this is kind of a breaking change for Python 3.6 users but everyone needs to upgrade at some point.

@caseyjhol
Copy link
Contributor Author

caseyjhol commented Sep 29, 2023

That makes sense to me (to lock CSS support to Python 3.7 starting in v0.10.0). If we get someone requesting Python 3.6 CSS inlining support, we can reconsider a different approach.

If we want to be really granular about it, we could release my CSS fixes in v0.9.2 (so Python 3.6 users could still use that version for CSS support). And then we could follow-up with a css-inline upgrade in v0.10.0. I think that probably makes sense. I'll split that out into a separate PR (#46).

@caseyjhol
Copy link
Contributor Author

@FelixSchwarz Any thoughts here? These are the last items preventing from moving forward with a prototype for some users. Thanks!

@FelixSchwarz
Copy link
Owner

Your branch contains some "wip-like" commits. I tried to extract the meaningful changes but I'm always getting some test failures locally. May I ask you to rebase/squash your commits on top of the latest main? I don't have any objections to code, so this just about getting your changes to apply (and a cleaned-up commit history).

@caseyjhol
Copy link
Contributor Author

Sure - can clean those up now. I'm used to doing a bunch of commits and then just doing a squash/merge.

@FelixSchwarz FelixSchwarz merged commit 84c495d into FelixSchwarz:main Nov 17, 2023
9 checks passed
@FelixSchwarz
Copy link
Owner

Thank you. Are there more features you would like to add or should we just release version 0.10?

@caseyjhol
Copy link
Contributor Author

I think that's it for now - I'm good with a 0.10.0 release. Thanks!

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.

Child selector styles not being applied
3 participants