-
-
Notifications
You must be signed in to change notification settings - Fork 304
feat: React Emails > default template #3194
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
base: cynthia/react-email
Are you sure you want to change the base?
feat: React Emails > default template #3194
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Comment |
"isCloud" to isCloud, | ||
"instanceQualifier" to if (isCloud) tolgeeProperties.appName else tolgeeProperties.frontEndUrl.intoQualifier(), | ||
"instanceUrl" to tolgeeProperties.frontEndUrl, | ||
"instanceUrl" to "http://localhost:8080", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing that it makes me wonder if the issues you were experiencing are that in development the frontEndUrl points to the Vite server and it isn't configured to proxy /static/*
to the backend... That'd explain it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some feedback for consideration 😄
emailService.sendEmailTemplate( | ||
recipient = params.to, | ||
subject = params.subject, | ||
template = params.templateName ?: "default", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I recon my original idea was to eventually deprecate that component; i.e. new code would use the new email system while legacy code can still use this until it is eventually migrated to use a dedicated template. Not sure templateName is needed 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed to wrap all existing emails in a default template and just drop the legacy email text into it. Since the new email designs aren’t ready yet, this lets us still use the feature in the meantime 😀
"instanceQualifier" to if (isCloud) tolgeeProperties.appName else tolgeeProperties.frontEndUrl.intoQualifier(), | ||
"instanceUrl" to tolgeeProperties.frontEndUrl, | ||
"instanceQualifier" to if (isCloud) tolgeeProperties.appName else backendUrl.intoQualifier(), | ||
"backendUrl" to backendUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about that... Especially wrt the instance qualifier. The fallback to using the domain name people are using to access Tolgee is likely a lot more recognizable than whatever internal URL is used.
Problems in development environment are likely related to the fact I completely overlooked that and didn't add the appropriate proxy config to Vite 😅
|
||
val html = templateEngine.process(template, context) | ||
val subject = extractEmailTitle(html) | ||
val subject = subject ?: extractEmailTitle(html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Setting a subject would break internationalization of emails. I guess this is needed for the backcompat layer, but... not ideal as it makes the service contract loose here :(
I don't think the template allows the subject being a variable directly but that could be mitigated with a passthrough i18n string; i.e. email.legacy.subject-passthrough = "{subject}"
defaultValue="Welcome and thank you for creating an account!" | ||
/>{' '} | ||
</If.Then> | ||
<If.Else></If.Else> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: If.Else
isn't strictly mandatory; looking back at the HACKING.md note I see where it comes from but I more or less wanted to express that bare JSX as children of If
isn't great because it's hard to tell what part of an if-then-else
we're working with.
Empty else doesn't hurt, but omitting it is fine imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am afraid the reason i put it there is, that it actually didn't work without <If.Else></If.Else>
🤷♂️ (I guess <If.Else />
shoud be good too) .. I don't know why, I was too lazy to find out 😁
btw this template is not used anyway, so it might be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially included it as a showcase/"real-world" demo (and was the template I used while prototyping back then); but it can indeed be ditched 😅
Weird that the Else branch is required; I know for sure it didn't use to be but I might've enforced it when refactoring at some point. Not very important anyway... :)
/> | ||
</Text> | ||
<Text> | ||
<If condition="${isSignup}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure using the same template for registration and email changes is advisable (given the advanced templating system now); subject of the email might be ambiguous and it limits the extensibility of the registration email (e.g. adding additional sections such as getting started guides and what not).
The wording of the confirm-email-text
is a bit confusing too; people aren't "starting to use" Tolgee but rather "changing their email" and I think the email body should reflect that.
Related to the 1st point email change confirmation (and other important emails) should perhaps contain warnings and advise caution regarding phishing campaigns and what to do in case of suspicious activity (use MFA, change passwords, etc).
b96f7dd
to
8f5ab03
Compare
954b9e4
to
add5c96
Compare
8f5ab03
to
f2badad
Compare
f2badad
to
d229261
Compare
@dkrizan can you add screenshot of those emails before and after the change? |
This PR is stale because it has been open for 30 days with no activity. |
No description provided.