Closed MatheusBuss closed 2 years ago
damn @NeoVier this review is 🔥🔥
Thanks a lot for the feedback and compliments @lucca65 😁. No worries about the delay. I just have somethings to reply.
I was thinking about removing the /api
from the path. But doesn't this require some NGINX configuration to route to the backend instead of the frontend?
I have defined the font on the html body as nunito sans, I thought this would be enough. I'll have a look into this then.
The other points seem like really good improvements! I'll get on them.
I was thinking about removing the /api from the path. But doesn't this require some NGINX configuration to route to the backend instead of the frontend?
You are right it will require us to add another rule to our nginx redirects. We have to update our nginx configuration for sure to include another rewrite
I have defined the font on the html body as nunito sans, I thought this would be enough. I'll have a look into this then.
Could be something safari related, it wouldn't be the first time this happens
I have defined the font on the html body as nunito sans, I thought this would be enough. I'll have a look into this then.
Although there is a declaration where you make body
's font nunito
, there isn't a link
tag that includes that font. What happens is that, if the user's computer has the nunito
font installed, it will use that, otherwise it will use their default sans-serif
font (which is a type of font, not a specific one) - that's because the declaration is 'nunito', sans-serif
, and the browser will try them in order.
To include the font, you need these tags in the head
(they come from https://fonts.google.com):
<link href="https://fonts.googleapis.com/css2?family=Nunito+Sans:ital,wght@0,200;0,300;0,400;0,600;0,700;0,800;0,900;1,200;1,300;1,400;1,600;1,700;1,800;1,900&display=swap" rel="stylesheet">
<link rel="preconnect" href="https://fonts.googleapis.com">
Also, you're using only nunito, sans-serif
. I'd advise using the same as we use on the frontend:
body {
font-family: 'Nunito Sans', 'Lato', 'San Francisco', 'Helvetica', 'Verdana', sans-serif;
}
By the way, it might be an issue to use google fonts (there were some problems with GDPR not too long ago: https://www.theregister.com/2022/01/31/website_fine_google_fonts_gdpr/). The ideal way would be for us to download the fonts and host them ourselves, but this is the way we're doing things for now
@lucca65 I've created #286 to address some of your points. The only one remaining should be the NGINX configuration. Should we host it under community.cambiatus.io/email
or something else such as community.cambiatus.io/unsubscribe
?
About special permissions on this page, this should already be handled under both the unsubcribe controller and the GraphQL authentication. I don't think NGINX will have any say about this, right? Perhaps we could move this special authentication from the unsubscribe controller to a special route on our router.
I think to avoid confusion we should add a pipeline to all email interactions. https://staging.cambiatus.io/mailer/${route}
or even https://staging.cambiatus.io/m/${route}
What issue does this PR close
Closes #261
Changes Proposed ( a list of new changes introduced by this PR)
Create page for handling email unsubscription without needing password identification
How to test ( a list of instructions on how to test this PR)
Locally with a transfer email
iex -S mix phx.server
{:ok, t} = Cambiatus.Commune.get_transfer(#{transfer_id})
CambiatusWeb.Email.transfer(t)
localhost:4000/dev/mailbox
token
to unsubscribe from the unsubscribe link at the end of the emailhttp://localhost:4000/api/unsubscribe?token=#{token}
Alternatively I'll be deploying this on staging to test there as well