markets / maily

📫 Rails Engine to preview emails in the browser
https://rubygems.org/gems/maily
MIT License
707 stars 31 forks source link

[WIP] Allow to add multiple hooks versions per mail #31

Closed pcriv closed 6 years ago

pcriv commented 6 years ago

Description

Closes #9

Hi @markets, i took a shot at implementing issue #9, tried a few of the approaches and went with the one that felt easier to integrate with the existing code.

This PR is mostly to show you my approach so i'm totally open to completely change (or close) it, if it doesn't fit the standards or style of your project.

Anyway i hope at least it's useful for some brainstorming about the feature :)

Now, This is how you define different hook versions register_hook_version method:

mailer.register_hook(:registration_accepted, user)
mailer.register_hook_version(:registration_accepted, "Test 1", User.last)
mailer.register_hook_version(:registration_accepted, "Test 2", User.second)

Almost the same signature of the register_hook one, but takes the version as the second argument. I went with this approach bc i found some issues sending the version as a key parameter at the end.

Anyway, let me know your thoughts and if you think this approach or worth continuing on!

TODO:

markets commented 6 years ago

Hi @pablocrivella and thanks a lot for starting work on this feature! It could be really useful! great work :ok_hand:

About your implementation, I have some comments/considerations, as you said, at least to start a discussion:

(1) I'd go with option (a) defined in #9, to avoid new public API methods and for consistency with other options: description, template_path, ...

What kind of issues did you have with this approach?

(2) When I started thinking about this, I imagined the following UI: in the sidebar list only 1 version per email with some indicator showing that this email has different versions; and then in the show view, add links (or a select) to all versions for the given email.

Your current implementation treats all versions as different emails, and I'm not sure... I think would be nice to differentiate between emails and versions

(3) We still support Ruby 2.2, so we should try to avoid using &.

Thanks again Pablo! Let's discuss pros/cons about you proposal and my comments.

markets commented 3 years ago

hey @pcriv 👋🏼

We finally released this feature! See #49 if you want to know more details. Thanks for your initial and inspirational work!

Best, Marc