FelixSchwarz / mjml-python

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

Add mjml-accordion, mjml-carousel, mjml-head-html-attributes, mjml-hero, and mjml-social components #31

Closed caseyjhol closed 1 year ago

caseyjhol commented 1 year ago

Closes #26.

This adds:

Note: For some reason MJML upstream uses cell-spacing and cell-padding (with dashes) for mjml-accordion. assert_same_html correctly considers these different from cellspacing and cellpadding. As a result, mj-accordion-expected.html requires some manual fixup after running the update-expected-html script.

FelixSchwarz commented 1 year ago

Note: For some reason MJML upstream uses cell-spacing and cell-padding (with dashes) for mjml-accordion.

I think this is very likely an upstream bug nobody noticed so far. Can you open a bug upstream?

Since I merged your "custom components" PR of course this PR has a lot of extra commits. If you could rebase this PR I can give you feedback if you like.

caseyjhol commented 1 year ago

Opened https://github.com/mjmlio/mjml/issues/2636 to address the cell-spacing/cell-padding issue.

caseyjhol commented 1 year ago

@FelixSchwarz I think this is ready for review (it'll probably be a few weeks before I have a chance to get around to the other components).

Also, do you know when the next release might be?

caseyjhol commented 1 year ago

Edit: there is a bug in MJML v4.13.0 that has been fixed, but is not yet included in any releases. Because of this, and the cell-spacing/cell-padding issue, I'm going to rebuild expected HTML using the latest mjml master branch, rather than the latest release.

@FelixSchwarz I added mj-social/mj-social-element components to this PR. However, there's a discrepancy with what MJML upstream outputs for padding surrounding the link text.

We're producing:

<td style="vertical-align:middle;padding:4px 4px 4px 0;">
    <a href="https://www.facebook.com/sharer/sharer.php?u=https://mjml.io/" style="color:#333333;font-size:15px;font-family:Ubuntu, Helvetica, Arial, sans-serif;line-height:22px;text-decoration:none;" target="_blank"> Facebook </a>
</td>

MJML upstream is producing:

<td style="vertical-align:middle;">
    <a href="https://www.facebook.com/sharer/sharer.php?u=https://mjml.io/" style="color:#333333;font-size:15px;font-family:Ubuntu, Helvetica, Arial, sans-serif;line-height:22px;text-decoration:none;" target="_blank"> Facebook </a>
</td>

As far as I can tell, there should be padding surrounding this link, and it seems like MJML upstream is broken somehow. But perhaps the issue is on our end.

I opened https://github.com/mjmlio/mjml/issues/2649.

caseyjhol commented 1 year ago

@FelixSchwarz I've now added all missing components from #26.

caseyjhol commented 1 year ago

@FelixSchwarz Any thoughts on the next release? I'm looking into implementing this and it would be great to have some of the new features available in an official release. Thanks!

caseyjhol commented 1 year ago

@FelixSchwarz Fixed merge conflicts and linting errors after ruff was merged in.

FelixSchwarz commented 1 year ago

Thank you very much for your contribution - I did a bit of cleanup on your commits (e.g. removing all the extra commits for custom components, merging some intermediate commits) and put this in a new PR (#38). If it is ok with you I'd merge your work as part of that other PR.

Are there any components in upstream mjml which you did not yet port to this repo? I'm asking because a full port of all elements could be a good reason to rename the repo (#33).

caseyjhol commented 1 year ago

Yea I'm fine with you just merging my stuff in in that other PR. As far as I can tell, all upstream components will be supported after this work is merged in.

FelixSchwarz commented 1 year ago

Thanks again for all your porting.