Email: Add a dynamic API email for new fundraisers #948
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces three new HTML email templates related to fundraising activities: Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
shared/emails/transactional/fundraiser-live-en.html(1 hunks)
🔇 Additional comments (1)
shared/emails/transactional/fundraiser-live-en.html (1)
704-718: LGTM: Proper implementation of unsubscribe functionality
The email template correctly implements unsubscribe functionality using SendGrid variables and follows email best practices by placing these options in the footer.
|
Visit the preview URL for this PR (updated for commit 52e60f6): https://si-admin-staging--pr948-sandino-fundraiser-s-vdk50s37.web.app (expires Mon, 09 Dec 2024 14:14:45 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
…fundraiser’s author and another to thank a contributor.
|
Email start of campaign: Template ID: d-a03378d2719a4e38b6c21af18be132c4 @andrashee of course, you should name the variables to your liking—I’ve marked them with { }. Depending on whether someone sets a goal, a conditional rule might be needed. This is a simple version of the email to start with, and we can always make it more advanced later. Visualized: |
ssandino
requested review from
andrashee,
mkue and
socialincome-dev
as code owners
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
shared/emails/transactional/fundraiser-end-en.html (3)
199-205: Consider responsive image dimensionsThe logo image has fixed dimensions which might not scale well on all devices.
-width="250" -style="display: block; width: 250px; max-width: 250px; min-width: 250px" +width="100%" +style="display: block; width: 100%; max-width: 250px"
450-455: Update hardcoded LinkedIn profile URLThe signature section contains a hardcoded LinkedIn profile URL. Consider making this configurable.
-<a href="https://www.linkedin.com/in/aurélie-schmiedlin/" +<a href="{sender_linkedin_url}"
122-177: Optimize preheader lengthThe preheader section contains excessive whitespace characters. Consider reducing them while maintaining email client compatibility.
-<!-- Current long sequence of whitespace characters --> +<!-- Reduce to approximately 50-100 characters for better compatibility -->shared/emails/transactional/fundraiser-thanks-contributor-en.html (1)
359-378: Improve video thumbnail accessibilityThe video thumbnail uses a GIF format which may be large and doesn't provide playback controls.
Consider:
- Using a static image with a play button overlay
- Adding aria-label for better screen reader support
- Including video duration in the alt text
<img class="max-width" border="0" + aria-label="Click to watch testimonial video from Social Income recipients" - alt="Video of the impact of Social Income" + alt="Video testimonial about Social Income's impact (2:30)"shared/emails/transactional/fundraiser-start-en.html (2)
535-535: Consider making support email configurableThe support email address is hardcoded. Consider making it a dynamic placeholder for easier maintenance.
- <a href="mailto:[email protected]" + <a href="mailto:{support_email}"
237-238: Consider using relative units for font sizesUsing pixel units for font sizes may impact accessibility. Consider using relative units (rem/em) for better scalability.
- font-size: 42px; - line-height: 52px; + font-size: 2.625rem; + line-height: 3.25rem; - font-size: 22px; + font-size: 1.375rem;Also applies to: 371-372
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
shared/emails/transactional/fundraiser-end-en.html(1 hunks)shared/emails/transactional/fundraiser-start-en.html(1 hunks)shared/emails/transactional/fundraiser-thanks-contributor-en.html(1 hunks)
🔇 Additional comments (5)
shared/emails/transactional/fundraiser-end-en.html (2)
606-618: Verify unsubscribe functionality
The template uses SendGrid's unsubscribe variables. Ensure these are properly configured in SendGrid:
{{{unsubscribe_preferences}}}{{{unsubscribe}}}
✅ Verification successful
Unsubscribe functionality is consistently implemented
The search results show that both {{{unsubscribe_preferences}}} and {{{unsubscribe}}} variables are consistently used across all email templates in the repository, including newsletters, transactional emails, and base templates. The implementation follows the same pattern with proper target and rel attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent unsubscribe implementation across templates
rg -A 1 '\{\{\{unsubscribe'
Length of output: 12206
244-244: Verify template variable handling
The template uses several dynamic variables that need proper fallback handling:
{Goal Reached! 🎉.}(line 244){amount} {currency}(line 277){fundraiser title}(line 323)
Also applies to: 277-277, 323-325
✅ Verification successful
Template variables are consistently used across email templates
The verification shows that the template variables are properly used across email templates with consistent patterns:
{amount}and{currency}are used together for monetary values{fundraiser title}is used for campaign names{Goal Reached! 🎉.}is a conditional message
The variables follow SendGrid's templating syntax and have proper fallback handling through the template engine.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent variable usage across email templates
rg -A 1 '{amount}|{currency}|{fundraiser title}|{Goal Reached!'
Length of output: 194
Script:
#!/bin/bash
# Search for template variables in email templates, escaping special characters
rg -A 1 "\{amount\}|\{currency\}|\{fundraiser title\}|\{Goal Reached"
# Also search specifically in the fundraiser-end template
cat shared/emails/transactional/fundraiser-end-en.html
Length of output: 29724
shared/emails/transactional/fundraiser-thanks-contributor-en.html (2)
584-596: Verify unsubscribe functionality
The template uses SendGrid's unsubscribe variables. Ensure these are properly configured in SendGrid.
11-21: Verify font loading reliability
The custom font 'Unica77' is loaded from an S3 bucket. Ensure there's a fallback system font stack in case the custom font fails to load.
Add fallback fonts:
-font-family: 'Unica77';
+font-family: 'Unica77', -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen-Sans, Ubuntu, Cantarell, sans-serif;✅ Verification successful
Let me check if there are any existing fallback fonts defined in the codebase.
Font loading is properly configured with fallbacks
The custom font 'Unica77' is already configured with appropriate system font fallbacks across email templates:
- Font files are accessible via HTTPS
- All font-family declarations include fallback fonts:
'Unica77', Helvetica, Arial, sans-serif
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the font URLs are accessible
curl -I https://si-fonts.s3.eu-west-1.amazonaws.com/Unica77LLWeb-Regular.woff2
curl -I https://si-fonts.s3.eu-west-1.amazonaws.com/Unica77LLSub-Medium.woff2
Length of output: 1716
Script:
#!/bin/bash
# Search for font-family declarations in HTML files
rg -t html "font-family" -B1 -A1
Length of output: 69087
shared/emails/transactional/fundraiser-start-en.html (1)
458-458: Ensure URL slug sanitization
The share URL uses a dynamic {slug} placeholder. Ensure proper sanitization to prevent URL injection.
✅ Verification successful
URL slug validation is properly implemented
The codebase has a validation pattern that ensures slugs follow a safe format using the regex /^[a-z0-9]+(?:-[a-z0-9]+)*$/, which only allows lowercase alphanumeric characters and hyphens, preventing URL injection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL slug sanitization in the codebase
rg -A 5 "sanitize.*slug|slug.*sanitize"
Length of output: 41
Script:
#!/bin/bash
# Search for slug-related code patterns
ast-grep --pattern 'slug = $_'
# Search for URL generation or validation patterns
rg -A 5 "createUrl|validateUrl|generateUrl|slugify"
# Search for campaign-related URL handling
rg -A 5 "campaign.*url|url.*campaign"
Length of output: 126
Script:
#!/bin/bash
# Look for email template handling or campaign URL generation
rg -A 5 "template.*campaign|campaign.*template"
# Search for any slug handling in JavaScript/TypeScript files
fd -e js -e ts --exec rg -A 5 "slug" {}
# Look for campaign-related functions or classes
ast-grep --pattern 'function $_(campaign|Campaign)'
Length of output: 833
| <a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> you | ||
| <span style="color: #01579b">convinced {total-contributors} to donate {amount} {currency}</span> for |
There was a problem hiding this comment.
Fix hardcoded link in fundraiser title
The fundraiser title link points to "https://google.ch" which appears to be a placeholder.
-<a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a>
+<a href="{fundraiser_url}" target="_blank" rel="noopener noreferrer">{fundraiser title}</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> you | |
| <span style="color: #01579b">convinced {total-contributors} to donate {amount} {currency}</span> for | |
| <a href="{fundraiser_url}" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> you | |
| <span style="color: #01579b">convinced {total-contributors} to donate {amount} {currency}</span> for |
| opacity: 0; | ||
| " | ||
| > | ||
| Your donation for “{fundraiser title}” organized by {fundraiser author}. It has been received successfully. Thank |
There was a problem hiding this comment.
Fix template variable syntax
The preheader text contains unescaped quotes around the template variable.
-Your donation for "{fundraiser title}" organized by {fundraiser author}. It has been received successfully. Thank
+Your donation for {fundraiser title} organized by {fundraiser author}. It has been received successfully. Thank📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Your donation for “{fundraiser title}” organized by {fundraiser author}. It has been received successfully. Thank | |
| Your donation for {fundraiser title} organized by {fundraiser author}. It has been received successfully. Thank |
| <a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> by | ||
| <span style="color: #01579b">{fundraiser author}</span> and helps bring hope to many people living in |
There was a problem hiding this comment.
Security: Verify URL construction
The Google URL is hardcoded. This should likely be a dynamic link to the fundraiser page.
-<a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> by
+<a href="{fundraiser_url}" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> by📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://google.ch" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> by | |
| <span style="color: #01579b">{fundraiser author}</span> and helps bring hope to many people living in | |
| <a href="{fundraiser_url}" target="_blank" rel="noopener noreferrer">{fundraiser title}</a> by | |
| <span style="color: #01579b">{fundraiser author}</span> and helps bring hope to many people living in |
| alt="Video of the impact of Social Income" | ||
| data-proportionally-constrained="true" | ||
| data-responsive="true" | ||
| src="https://cdn.mcauto-images-production.sendgrid.net/044784a789c4863a/a8cfc455-8d53-439c-9905-1887176cee83/600x338.gif" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the video preview GIF
Large GIF files can significantly increase email size. Consider using a static image with a play button overlay instead.
- src="https://cdn.mcauto-images-production.sendgrid.net/044784a789c4863a/a8cfc455-8d53-439c-9905-1887176cee83/600x338.gif"
+ src="https://cdn.mcauto-images-production.sendgrid.net/[static-image-path].jpg"Committable suggestion skipped: line range outside the PR's diff.
| <tr> | ||
| <td align="center" bgcolor="#f9f8f3" style="padding: 10px"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%" style="max-width: 600px"> | ||
| <tr> | ||
| <td | ||
| bgcolor="#FAC800" | ||
| style=" | ||
| background-color: #f0e08c; | ||
| padding: 12px 15px; | ||
| border-radius: 10px; | ||
| font-family: 'Unica77', Helvetica, Arial, sans-serif; | ||
| font-size: 24px; | ||
| line-height: 32px; | ||
| text-align: center; | ||
| " | ||
| > | ||
| <p | ||
| class="feature" | ||
| style=" | ||
| margin: 0; | ||
| font-family: 'Unica77', Helvetica, Arial, sans-serif; | ||
| font-weight: 400; | ||
| font-size: 18px; | ||
| line-height: 26px; | ||
| color: #59481c; | ||
| " | ||
| > | ||
| {n} days left to fundraise | ||
| </p> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> | ||
| <!-- end banner --> | ||
| <!-- start banner without goal --> | ||
| <tr> | ||
| <td align="center" bgcolor="#f9f8f3" style="padding: 10px"> | ||
| <table border="0" cellpadding="0" cellspacing="0" width="100%" style="max-width: 600px"> | ||
| <tr> | ||
| <td | ||
| bgcolor="#FAC800" | ||
| style=" | ||
| background-color: #f0e08c; | ||
| padding: 12px 15px; | ||
| border-radius: 10px; | ||
| font-family: 'Unica77', Helvetica, Arial, sans-serif; | ||
| font-size: 24px; | ||
| line-height: 32px; | ||
| text-align: center; | ||
| " | ||
| > | ||
| <p | ||
| class="feature" | ||
| style=" | ||
| margin: 0; | ||
| font-family: 'Unica77', Helvetica, Arial, sans-serif; | ||
| font-weight: 400; | ||
| font-size: 18px; | ||
| line-height: 26px; | ||
| color: #59481c; | ||
| " | ||
| > | ||
| {n} days left to fundraise towards your goal of {currency} {amount} | ||
| </p> | ||
| </td> | ||
| </tr> | ||
| </table> | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
Implement conditional rendering for banner sections
The template contains two identical banner sections - one with goal amount and one without. These should be conditionally rendered server-side to avoid potential display issues.
Implement server-side conditional rendering:
- <!-- start banner with goal -->
- <tr>
- <td align="center" bgcolor="#f9f8f3" style="padding: 10px">
- ...
- </td>
- </tr>
- <!-- end banner -->
- <!-- start banner without goal -->
- <tr>
- <td align="center" bgcolor="#f9f8f3" style="padding: 10px">
- ...
- </td>
- </tr>
+ {{#if hasGoal}}
+ <!-- start banner with goal -->
+ <tr>
+ <td align="center" bgcolor="#f9f8f3" style="padding: 10px">
+ ...
+ </td>
+ </tr>
+ {{else}}
+ <!-- start banner without goal -->
+ <tr>
+ <td align="center" bgcolor="#f9f8f3" style="padding: 10px">
+ ...
+ </td>
+ </tr>
+ {{/if}}Committable suggestion skipped: line range outside the PR's diff.
To-Do:
1. Decide how to handle the two variants (with or without a goal): separate emails or conditional logic.
2. Decide whether to include an unsubscribe option for this important email.
For testing, you can access the email in SendGrid using the Template ID: d-a03378d2719a4e38b6c21af18be132c4.
Summary by CodeRabbit
fundraiser-end-en.html: Acknowledges the conclusion of a fundraising campaign.fundraiser-start-en.html: Initiates fundraising activities with engaging content.fundraiser-thanks-contributor-en.html: Thanks contributors for their donations and highlights impact.