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

mj-section #10

Closed
Tracked by #1
SebastianStehle opened this issue Mar 13, 2022 · 12 comments · Fixed by #31
Closed
Tracked by #1

mj-section #10

SebastianStehle opened this issue Mar 13, 2022 · 12 comments · Fixed by #31
Assignees

Comments

@SebastianStehle
Copy link
Owner

No description provided.

@SebastianStehle SebastianStehle mentioned this issue Mar 13, 2022
43 tasks
@LiamRiddell LiamRiddell self-assigned this Mar 13, 2022
@LiamRiddell
Copy link
Collaborator

I'll pick this one up. 👍

@SebastianStehle
Copy link
Owner Author

Awesome. I started with it in the other project. ConsoleApp2. Perhaps it helps. It is the only reason to keep this one.

@LiamRiddell LiamRiddell linked a pull request Mar 13, 2022 that will close this issue
@SebastianStehle
Copy link
Owner Author

Do you also handle mj-column and mj-group? They seem to be very similar.

@LiamRiddell
Copy link
Collaborator

I can try, but fighting with mj-section at the minute 😆. I've published a initial draft without background image support. I'm just tidying up the mj-button and mj-text and I'll work on getting them merged. Then return back to section.

@SebastianStehle
Copy link
Owner Author

Sure, we do not have to be done today :D ... I am so excited that about your great response to my feedback and the progress so far :)

@LiamRiddell
Copy link
Collaborator

Sure, we do not have to be done today :D ... I am so excited that about your great response to my feedback and the progress so far :)

Haha, I think we've made good progress today. It will slow down during week as I'm in full-time work. However, feel free to pick up anything of mine. I have no local changes right now and everything is committed to branches.

We do still have the three hardest components left!

  1. mj-section
  2. mj-column
  3. mj-group

I'm thinking for mj-section we drop background-image support for now and build it once we've got everything working together.

I would of liked to get it done today :D

@SebastianStehle
Copy link
Owner Author

Haha, I think we've made good progress today. It will slow down during week as I'm in full-time work.

Me too, but we use https://github.com/notifo-io/notifo at work and my boss is on paternal leave and I can do what I want. But I also have another interesting tasks that I want to work on.

@LiamRiddell
Copy link
Collaborator

Me too, but we use https://github.com/notifo-io/notifo at work and my boss is on paternal leave and I can do what I want. But I also have another interesting tasks that I want to work on.

Us developers always have too many projects we want to work on at once :D

Notifo

It looks really cool! I hope we can get MJML.NET v2 in there some day!

@SebastianStehle
Copy link
Owner Author

It looks really cool! I hope we can get MJML.NET v2 in there some day!

Thanks, I use mjml over a node port but the problem is performance.

I have to combine the emails at runtime, because you could have N notifications that are bundled together to a single email. So you basically have a loop.

My preferred attempt would be to just create templates with fluid/liquid and I tried it, but it is too slow.
In my tests it took around 30 to 100ms, which is very long. So I created this very ugly workaround with comments:

https://github.com/notifo-io/notifo/blob/main/backend/src/Notifo.Domain/Channels/Email/Formatting/DefaultHtml.mjml

For each type of notification you create one subtemplate and then the html fragments are combined together. But what happens when there is another element in the notification?

You already have

  • Notification
  • Notification with button
  • Notification with button and image
  • Notification with image

So if the notification get another element (lets say a badge or whatever) it explods.

@LiamRiddell
Copy link
Collaborator

@SebastianStehle I'm running short on time this week so sorry for lack of commits on my end. I'll try and get some time to get some tests in the mj-section and hopefully the version 1 implementation (no background support).

@SebastianStehle
Copy link
Owner Author

No problem. I had a lot of time this week, because my boss is on parental leave and mjml provides value for the company as well.

I added the amario template to the test runner and it looks pretty good. There seems to be an issue with the mj-columns that have an explicit width of 50% for example. But otherwise it looks good.

@LiamRiddell
Copy link
Collaborator

No problem. I had a lot of time this week, because my boss is on parental leave and mjml provides value for the company as well.

I added the amario template to the test runner and it looks pretty good. There seems to be an issue with the mj-columns that have an explicit width of 50% for example. But otherwise it looks good.

Fantastic (whoops, on my end with columns), some pictures would be cool! I'm gonna create conditionals branch and change all to renderer.Content("...").

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 a pull request may close this issue.

2 participants