search

FelixSchwarz / mjml-python

Python implementation for MJML - a framework that makes responsive-email easy
MIT License
73 stars 16 forks source link

Adds support for mj-font, mj-preview and mj-raw #8

Closed barsch closed 3 years ago

barsch commented 3 years ago

tackles issue #7 - all tags have at least one test case - mj-raw is a bit ugly but couldn't find a better solution

May I suggest to use black as automatic code formatter - it was hard for me to emulate your code style ;)

barsch commented 3 years ago

Two more issues:

* Please squash the changes to `.gitignore`. These are really trivial. Often I just write something like ".gitignore: ignore eclipse project and .pyc files".

* "fix not working error message" needs a better commit message. Basically your issue text from #7 (comment) with slight adaptation would be fine.

done

If you start using the Python port in production I'd be very interested if you see any issues/what should be improved (and maybe what works great). Also how did you came across this project?

I hate to have a node based server running just for generating my newsletters - I tested it so far with our newsletters - not looking very good atm because of missing inline styles and the small issues I reported

May I suggest to use black as automatic code formatter - it was hard for me to emulate your code style ;) Well, so far this was just a quick thing for one person to solve a customer's problem so I did not care much about a fixed coding style as it might be shared in the Python community. However if this becomes an actual open source project with multiple contributors I guess I'll have to adapt a bit :-)

the nice thing about automated code formatters is you don't have to adapt - you save your python file and its just does it magic - sure one needs to live with the style its generates but at least it completely eliminates any style discussions because its the same for everyone - I basically don't care about formatting at all anymore which took so much time away from actual programming especially if it is a multi user project ...

FelixSchwarz commented 3 years ago

As mentioned in the review comments I'll merge this PR as-is. The PR enhances the status quo and the "innerXML part" should not prevent merging this.

Again: Thank you very much for your contributions.

I tested it so far with our newsletters - not looking very good atm because of missing inline styles and the small issues I reported

Do you plan to implement inline styles next? Do you need <mj-style inline="inline"> or generic inlining? (or both)