GrapesJS / mjml

Newsletter Builder with MJML components in GrapesJS
http://grapesjs.com/demo-mjml.html
BSD 3-Clause "New" or "Revised" License
637 stars 227 forks source link

MJML v4 0: Could it work? #73

Closed nojacko closed 5 years ago

nojacko commented 6 years ago

I'm looking to use MJML v4 with Grapes. MJML v4 doesn't support browser rendering so an API needs to be used. I've set up an API to render the MJML so that's fine.

The problem seems to be with having an asynchronous rendering of the HTML. Has anyone been able to work it or know if it's possible?

kickbk commented 6 years ago

related to #65 - would be great to have this working somehow.

nojacko commented 6 years ago

I did try and rendered parts on the server but Grapes expects things to be done synchronously so the render happened after Grapes worked it's magic. It seems like the rendering parts of Grapes would need to be updated to support callbacks or promises.

nojacko commented 6 years ago

Hey @artf. Sorry for the mention. Do you have any ideas about this?

artf commented 6 years ago

Well the only way I see is to extend the render function as already done here and implement there asynchronous rendering

nojacko commented 6 years ago

I did try that but it seems that whatever's calling the render function in Grapes is expecting it to be completed synchronously and carries on doing other things that really require the render to have completed. Because of this it breaks. "it breaks" isn't much help. I'll try get a demo going when I've got a moment.

bures commented 5 years ago

I have another solution. Look here: https://github.com/bures/mjml4-in-browser

The trick is to mock nodejs-specific modules in webpack. It's not a complete solution because certain options to MJML (e.g. to minify it) or MJML feature (e.g. mj-include) break it. However, for the common case in web-browser, it works. Anyway, mj-include makes no sense in web-browser settings.

Any chance now that you would upgrade grapesjs-mjml to MJML v4.2?

Thanks.

bures commented 5 years ago

Here the NPM package suitable for inclusion in webpack build: https://www.npmjs.com/package/mjml4-in-browser

artf commented 5 years ago

Thanks @bures cool to see your solution to make it work in the browser.

Any chance now that you would upgrade grapesjs-mjml to MJML v4.2?

To be honest, I'd like to work on other stuff right now, but hey, if someone need the v4, you can do it... You have the API of grapesjs and also the code of grapesjs-mjml...

bures commented 5 years ago

Not sure that I wanted to hear this. :-) I'm already too busy with Mailtrain v2 (https://github.com/Mailtrain-org/mailtrain/tree/development). So I guess the upgrade of grapesjs-mjml will have to wait.

artf commented 5 years ago

Yeah sure, mine was an invitation to anyone interested in porting this plugin up to MJML v4

CimattiConsulting commented 5 years ago

I updated mjml to version 4 but many things stopped working, I leave the link of the respository in case someone wants to give me help https://github.com/CimattiConsulting/grapesjs-mjml

bures commented 5 years ago

What exactly stopped working?

CimattiConsulting commented 5 years ago

these are the bugs that manifested updating the version of mjml and on which I haven't worked yet instead the bugs still present from the old version are:

bures commented 5 years ago

Thanks for the report. If you had time to look into those issues, it would be great. I'll then integrate it in Mailtrain. Thanks.

larsdecker commented 5 years ago

Any News about the mjml 4 Support for GrapesJS.

@CimattiConsulting Still working on that?

ttonyh commented 5 years ago

I think the best approach would be changing GrapesJs to work with Promises in the "render" method. That way the MJML processing can happen on the server-side. Does anyone have any idea how much work that would take, and where in the source code to start?

Amandeepsinghghai commented 5 years ago

@nojacko the render method is being called here let rendered = view.render().el.

@artf any suggestions on how to go about making it work with a Promise/callback? Something similar to changing https://github.com/artf/grapesjs-mjml/blob/master/src/command-export-mjml.js#L81

if (htmlCode) {
  let mjml = getMjml();
  if(mjml.errors.length) {
    mjml.errors.forEach((err) => {
      console.warn(err.formattedMessage);
    });
  }
  htmlCode.setContent(mjml.html);
  //htmlCode.editor.setOption('lineWrapping', 1);
  htmlCode.editor.refresh();
}

to

if (htmlCode) {
  getMjml().then(({ html, errors }) => {
    htmlCode.setContent(html)
    htmlCode.editor.refresh()
    // Handle errors
  })
}
CimattiConsulting commented 5 years ago

@ttonyh The best approach is to use https://github.com/bures/mjml4-in-browser Communication with a web server would make it much slower

CimattiConsulting commented 5 years ago
  • Some fonts no longer work (You can see it in the default newsletter)
  • The width attribute of the mj-body component doesn't work

these are the bugs that manifested updating the version of mjml and on which I haven't worked yet instead the bugs still present from the old version are:

  • When the padding isn't specified it's assumed that it's 0px but it's actually 10px or 20px depending on the component
  • Once the color has been inserted into an attribute, it's impossible to remove it

@ashmna @artf @bures I've updated the bugs' list. If someone wants to help, but can't reproduce the problem he want to fix, tell me

larsdecker commented 5 years ago

@CimattiConsulting Where can we find the bug list? Maybe i can help to fix some bugs.

CimattiConsulting commented 5 years ago

@larsdecker Work on https://github.com/CimattiConsulting/grapesjs-mjml Here you are:

these are the bugs that manifested updating the version of mjml and on which I haven't worked yet instead the bugs still present from the old version are:

larsdecker commented 5 years ago

@CimattiConsulting

I have fix the spacer element on my fork (https://github.com/larsdecker/grapesjs-mjml) The width attribute seems to be working.

Did you know how we can deactivate the fonts that are not working? I think this comes directly from the grapesJS Core.

I now try to figure out, how we can fix the other bugs that are also exists in the old versions.

CimattiConsulting commented 5 years ago

@larsdecker

Thank you, The mj-body width attribute stops working when it exceeds 600px, it seems that in one of its children there is a max-width attribute for some reason.

CimattiConsulting commented 5 years ago

@larsdecker

if I find the time I will look at the font problem

larsdecker commented 5 years ago

For those who want to use GrapesJS with MJML 4, install it using the following command and give us feedback about bugs.

npm install github:larsdecker/grapesjs-mjml --save

artf commented 5 years ago

https://github.com/artf/grapesjs-mjml/pull/105