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

Feature/mjml mails #1255

Merged
merged 21 commits into from
Oct 20, 2023
Merged

Feature/mjml mails #1255

merged 21 commits into from
Oct 20, 2023

Conversation

Samuelfaure
Copy link
Contributor

@Samuelfaure Samuelfaure commented Sep 27, 2023

fix https://linear.app/pole-api/issue/API-757/permettre-a-dorine-de-pre-visualiser-les-emails
fix https://linear.app/pole-api/issue/API-756/migrer-les-mails-api-entreprise-vers-admin

Review: commit-par-commit

Reste:

A réparer post tests sandbox:

  • images
  • police

@Samuelfaure Samuelfaure requested review from skelz0r and Un3x September 27, 2023 14:45
@linear
Copy link

linear bot commented Sep 27, 2023

API-756 Migrer les mails API Entreprise vers admin

Il s'agit de rapatrier tous les modèles de mailjet dans le code en erb/html/mjml (à voir loic.delmairebeta.gouv.fr un avis ?)

réutiliser au maximum le code (il y a de nombreux modules/sections réutilisées)

API-757 Permettre à Dorine de pré-visualiser les emails

lors du développement dorine.lambinetbeta.gouv.fr doit pouvoir visualiser les emails sur lesquels elle travaille via :

Conception via : https://demo.mailjet.com/

Visualisation via Rails tout simplement http://localhost:3000/rails/mailers

Visualisation sur une PR, on clonera en local la branche et on check à la main

@Samuelfaure Samuelfaure force-pushed the feature/mjml_mails branch 2 times, most recently from 934fc60 to d9b4c9c Compare September 27, 2023 14:52
Copy link
Member

@skelz0r skelz0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui c'est good pour moi.

Enjoy pour la suite :}

Copy link
Contributor

@Un3x Un3x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis ok, j'admets avoir relu un peu de travers cela dit.

@Samuelfaure Samuelfaure force-pushed the feature/mjml_mails branch 2 times, most recently from 181a794 to 93952eb Compare October 2, 2023 12:39
@Samuelfaure Samuelfaure force-pushed the feature/mjml_mails branch 4 times, most recently from ba89801 to e147b4a Compare October 3, 2023 13:49
@Samuelfaure Samuelfaure changed the title [Draft] Feature/mjml mails Feature/mjml mails Oct 4, 2023
@Samuelfaure Samuelfaure requested review from skelz0r and Un3x October 4, 2023 10:35
Copy link
Member

@skelz0r skelz0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C’est un peu indigeste à lire, parce qu’il y a pas mal de choses qui:

  1. Ne servent pas
  2. Ne sont pas DRY

D’ailleurs des erreurs se sont glissés certainement à cause de ça:

Il faut que cela soit maintenable et iso avec les autres vues, pour cela il faut:

  1. Introduire des composants et des classes
  2. Introduire du i18n

Je te conseille de d’abord faire 1. (sur une vue), puis sur toutes les vues, puis 2. viendra tout seul (si tu supprime les trucs dégueu avec du span qui changent seulement la taille de police)

Pour les composants, on voit qu’il y a de commun:

  • Un subheader (qui change de couleur en fonction du type)
  • Du texte raw
  • Des CtA (2 types, pourquoi d’ailleurs ..? imo faut simplifier)
  • Du bloc fond gris
  • La liste des scopes

Tu peux ici faire des partials avec yield (ex: https://stackoverflow.com/questions/2951105/rails-render-partial-with-block)

Pour les classes, je t’invite à lire https://documentation.mjml.io/#mj-style

Pour ce qui ne sert pas, liste non exhaustive:

  • Les padding 0
  • Les data-testid
  • Les class

Au passage, les vues que tu as migrés en mjml sont peu lisibles, faudrait espacer/fixer des polices/normaliser (mais avec mjml-style global ça devrait aller mieux tout de suite).

@Samuelfaure
Copy link
Contributor Author

@skelz0r OK je pensais qu'on voulait se contenter d'exporter les mails et utiliser tels quels modulo la factorisation des grosses parties communes (header etc)

Je reprend du coup pour intégrer en composants plus finement

@skelz0r
Copy link
Member

skelz0r commented Oct 4, 2023

@skelz0r OK je pensais qu'on voulait se contenter d'exporter les mails et utiliser tels quels modulo la factorisation des grosses parties communes (header etc)

Je reprend du coup pour intégrer en composants plus finement

Oui c'est ce qu'on veut, mais cela n'empêche pas de rendre ça maintenable: le code on l'écrit une fois on le lit 100 fois. Clairement là chaque itération sera painfull as fuck, on ne veut clairement pas ça (tout du moins moi je ne veux pas ça, no matter what j'ouvre ça je vois que c'est pas DRY je refactor, autant que tu le fasses direct plutôt que de laisser du code comme ça).

@Samuelfaure Samuelfaure changed the title Feature/mjml mails [draft] Feature/mjml mails Oct 5, 2023
@Samuelfaure Samuelfaure force-pushed the feature/mjml_mails branch 2 times, most recently from 6a74381 to 0657b7c Compare October 10, 2023 08:52
Copy link
Member

@skelz0r skelz0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GG, sacré morceau 😅

J'ai fait des suggestions sur les br dans les titles (clairement ça va rendre n'importe quoi dans les clients emails. Déjà que c'est pas ouf dans un navigateur 🙄) mais globalement le code est OK pour moi.

L'étape d'après ici c'est de tester des vrais envois pour voir la gueule des emails. là où il faut faire attention:

  1. Les br dans les titles
  2. Les images attachments: je ne suis pas sûr que ça fonctionne.

Si tu fais des tests hésite pas à m'en envoyer btw.

<mj-text>
<h1>
<b>
<% if entity == "editeur" %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple remarque, entity_type aurait été mieux, entity on s'attend à un modèle.

Honnêtement ce n'est pas très grave, ça pourra se scout commiter au besoin

@skelz0r
Copy link
Member

skelz0r commented Oct 19, 2023

Et au fait j'ai approved, mais tant qu'un email n'est pas parti de la sandbox il ne faut pas merger imo

Co-authored-by: Delmaire Loïc <skelz0r@gmail.com>
Copy link
Contributor

@Un3x Un3x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Franchement bien joué c'est quand même un gros morceaux que tu viens de pondre là.

@Samuelfaure
Copy link
Contributor Author

@skelz0r les font ne fonctionnent pas -_- tu aurais une idée de pourquoi?

(les images non plus mais je vais faire des attachments)

@skelz0r
Copy link
Member

skelz0r commented Oct 19, 2023

@skelz0r les font ne fonctionnent pas -_- tu aurais une idée de pourquoi?

(les images non plus mais je vais faire des attachments)

Dans un de mes templates (le principal:

<mjml>
  <mj-head>
    <mj-font name="Open Sans" href="https://fonts.googleapis.com/css?family=Open+Sans:wght@300;400;500;600;700;800&display=swap'" />

je sais pas si ça marche mais personne ne s'est plaint 🤷

@skelz0r
Copy link
Member

skelz0r commented Oct 19, 2023

J'ai ça aussi dans mon template:

      <mj-all
        color="#1e1e1e"
        align="left"
        font-family="Open Sans, Helvetica Neue, Helvetica, Arial, sans-serif"
        font-size="14px"
      />

dans le head

@skelz0r
Copy link
Member

skelz0r commented Oct 19, 2023

Autre piste: reprend un des templates mailjet et voit comment ils ont fait

@Samuelfaure Samuelfaure force-pushed the feature/mjml_mails branch 2 times, most recently from 407a903 to 4e0c3ca Compare October 20, 2023 06:25
@Samuelfaure Samuelfaure merged commit 6514177 into develop Oct 20, 2023
5 checks passed
@Samuelfaure Samuelfaure deleted the feature/mjml_mails branch October 20, 2023 08:07
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 this pull request may close these issues.

3 participants