search

FelixSchwarz / mjml-python

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

Add more missing MJML components #28

Closed caseyjhol closed 1 year ago

caseyjhol commented 2 years ago

Address #26.

This PR contains the following components:

Tests have been added for each new component. *-expected.html was auto-generated using the update-expected-html script (with MJML v4.13.0).

caseyjhol commented 2 years ago

I think I'm going to keep this PR limited to just these components for now so it doesn't get too unruly.

caseyjhol commented 2 years ago

@FelixSchwarz Thoughts?

FelixSchwarz commented 2 years ago

I'm sorry, I had this on my todo list but I had a lot "to do" in the last month. I can probably not go through each commit today but I'm glad you pinged me. I'll try to finish the review asap.

caseyjhol commented 2 years ago

Thanks - no rush - I just wanted to make sure you saw the PR. Hoping to get around to more MJML work this week.

FelixSchwarz commented 2 years ago

ok, I had a first pass. In general this looks ok with a few stylistic issues. If you could improve some of the issues I'm really happy to merge your improvements.

PS: Don't forget to also extend the changelog :-)

caseyjhol commented 2 years ago

@FelixSchwarz Anything specific on the stylistic issues? (possibly you didn't hit "submit" on your code review?)

FelixSchwarz commented 2 years ago

Anything specific on the stylistic issues?

Just referring to the review comments. E.g. "f-strings inside f-strings" - not really a bug but I find that a bit annoying. However in the end I want to support everyone who is putting in the effort to extend this repo so I don't have a strong opinion on that.

caseyjhol commented 2 years ago

Happy to adjust to fit coding style better, but the review comments aren't showing up for me.

FelixSchwarz commented 2 years ago

I'm sorry - I was not aware that I had to "submit" the review before it was visible to you. Fixed now.

caseyjhol commented 1 year ago

@FelixSchwarz Just checking in - are there any more changes you'd like to see here?

FelixSchwarz commented 1 year ago

Just checking in - are there any more changes you'd like to see here

Actually I was not aware that you updated your code and was too deep into other work to notice. If you addressed my initial comments that should be ok. Thank you for pinging me.

I'll have a look later this week.

caseyjhol commented 1 year ago

@FelixSchwarz Updated to make missing_ok keyword-only.

FelixSchwarz commented 1 year ago

Thank you :-)