Ac/pm 21742/update confirmed to org email templates #6683
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6683 +/- ##
==========================================
- Coverage 53.65% 53.63% -0.02%
==========================================
Files 1926 1926
Lines 85720 85717 -3
Branches 7687 7686 -1
==========================================
- Hits 45989 45978 -11
- Misses 37958 37966 +8
Partials 1773 1773 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| headAnchorElement + | ||
| ` | ||
| </mj-text> | ||
There was a problem hiding this comment.
This change ensures we only add the text element when the component receives the respective input for the field. The current behavior adds an empty text element when no input is provided, which can impact spacing and style.
This change should not affect existing components, since the behavior remains the same for components that pass in data for that field.
| @@ -1,4 +1,12 @@ | |||
| const { BodyComponent } = require("mjml-core"); | |||
|
|
|||
| const BODY_TEXT_STYLES = ` | |||
There was a problem hiding this comment.
Hey folks, I got some feedback from Sukhleen that the font spacing doesn’t align with the design. I think this wasn’t caught earlier because the welcome email doesn’t have multi-line text, but the org confirmation email does.
I also noticed that the style from mj-bw-icon-row-text isn’t being applied to the text. I can’t get the styling to apply through the CSS class; it might be related to how MJML renders it.
The workaround is to inject the styling directly.
I’m open to any suggestions on how to handle this better.
|
Claude finished @JimmyVo16's task —— View job Review Complete ✅Todo List
Code Review: Organization Confirmation Email TemplatesOverall Assessment: The templates are well-implemented with proper security considerations. The code is ready to merge once the minor issues below are addressed. ✅ Security & ComplianceZero-Knowledge Principles: ✓ Pass
XSS Protection: ✓ Pass
Sensitive Data Handling: ✓ Pass
📝 Findings by Severityℹ️ Minor Issues1. Identical Text Templates ( Both Recommendation: Consider whether these should have different messaging based on organization type (Enterprise/Teams vs Families/Free), similar to how the HTML versions differ. If they're intentionally the same, this is fine. 🎨 Suggestions for Improvement2. Component Refactoring Quality ( The refactoring to add conditional rendering is well-done: ${this.getAttribute('text') ? `
<mj-text ... font-family="'Open Sans', Helvetica, Arial, sans-serif" ...>
${this.getAttribute('text')}
</mj-text>
` : ''}Observations:
The author's note about inline styling being a workaround due to CSS class application issues is understandable given MJML's rendering behavior. 3. Missing Unit Tests Per Bitwarden's development workflow, new features should include unit tests. Consider adding tests for:
4. Template Wiring PR description states: "There is no code for wiring this up." These templates will remain unused until integrated. Consider:
💭 Design Feedback NotesThe author mentioned mobile button sizing doesn't match design and will communicate to product. This is appropriate to handle outside the code review process. ✅ What Went Well
🔍 Additional Notes
Recommendation✅ APPROVE - This PR is ready to merge once the team confirms:
|
JimmyVo16
dismissed stale reviews from jrmccannon and BryanCunningham
via
ad6bedd
|
Originally, I was thinking of having a PR for each template, but they share some logic, so it’s easier to roll them into one PR. Sorry for the inconvenience, folks. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-21742
📔 Objective
This is to add the templates for the organization confirmation email.
There is no code for wiring this up.
Enterprise and Teams
Design
Families and Free
Design
📸 Screenshots
Enterprise and Teams
Mobile
Families and Free
Mobile
Note: I couldn’t get the mobile version to look like the design, mainly due to the button sizing. I’ll communicate this to product.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes