search

sighmon / mjml-rails

MJML + ERb/Haml/Slim view template
https://mjml.io
Other
301 stars 65 forks source link

[WIP] change mjml support version to be 4.0.x #23

Closed jipiboily closed 6 years ago

jipiboily commented 6 years ago

Oops...I did a bit of mistake, it was supposed to be against my own fork for now until I see if it's complete...

It's kinda hacky, but it's a start.

jipiboily commented 6 years ago

Closing for now, I need get my shit together. hehe

jipiboily commented 6 years ago

Build is passing on my repo, on Travis, but I need to get the test suite to pass locally...

https://github.com/metricswatch/mjml-rails/pull/1

jipiboily commented 6 years ago

@sighmon this is a WIP to support 4.0...but seems very brittle and hard-coded to use 4.0.0-beta.1.

Before I spend time on improving that:

  1. are you interested in such PR?
  2. what version of the gem should it be? I like when integration gems follow the version of what they integrate with...I think it makes it easier to spot the compatibility...but that's just my 2 cents.

Besides the fact that it's brittle and slightly hacky...anything else should be modified?

Cheers

sighmon commented 6 years ago

@jipiboily Thanks so much! I'm definitely keen on a PR.

If the update will break v3 MJML code, then we could bump the gem version to 4 and skip 3 as you suggest.

If you'd like to spend time cleaning up the file extensions to make them more Rails like, that'd be great. :-) Any other suggestions you have greatly received too.

Best, Simon.

jipiboily commented 6 years ago

In fact, for MJML itself, apparently v4.0 supports v3 syntax, will deprecate it in 4.1 and remove some stuff in 4.2.

So, I'm on the fence...we could make it work with both, v3 AND 4.1 and stick with a v4 version of the gem too.

Not sure to follow which file extensions you're talking about? mjml to be mjml.erb, for example?

The thing i find the most weird is that we need a format.mjml in the mailers...I would rather have the format.html pick up the mjml files. Thoughts on this? I could totally be lacking some context that makes this non-ideal or a really bad idea.

sighmon commented 6 years ago

In fact, for MJML itself, apparently v4.0 supports v3 syntax, will deprecate it in 4.1 and remove some stuff in 4.2.

Think I'd rather make it a small bump (mjml-rails v2.5) for the v4 support, and then a major version bump (mjml-rails v.4.0) when support for v3 is dropped. How does that sound?

Not sure to follow which file extensions you're talking about? mjml to be mjml.erb, for example?

I was mainly thinking of issues such as #14

The thing i find the most weird is that we need a format.mjml in the mailers...I would rather have the format.html pick up the mjml files. Thoughts on this? I could totally be lacking some context that makes this non-ideal or a really bad idea.

Yeah nice. It's been ages since I worked on this, and based it on another project - so you probably know it as well if not better than I do right now. :-) So feel free to improve it as you see fit.

jipiboily commented 6 years ago

I'm not sure it make sense to do a bump to 4.0 when mjml moves to 4.1. I think wrappers are better with matching versions. But that's just me. So, whatever you decide :)

I don't think I'll spend much time around the file extensions. At least, not in the near future.

What about we ship this as-is? I'll just change the gem version and you could publish it. If you're interested, I can help if I have access to the repo + Rubygems.

There will be a beta 2 very soon. So we could just improve on this.

Thoughts?

sighmon commented 6 years ago

@jipiboily Sorry for the radio silence - I've been flat out. I'd be happy to give you access to the repo, but I sign the gem, so publishing it would be difficult. But thanks so much for the updates, and keep submitting pull requests. (I'll try and get to them quicker next time)