-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat(ras-acc): update otp email template #3032
feat(ras-acc): update otp email template #3032
Conversation
I'm gonna mark this one ready for review despite more work needing to be done to match the designs to hopefully help make review a bit simpler. I should also note that there is a bug in the newsletters plugin (which it seems the main plugin emails rely on to generate email html) which causes the editor to force buttons to be centered. So our default OTP template will have a correctly left aligned button, but if you go into the post editor for the email template, make any changes, and save then the button will become centered. Similarly, if you send yourself a test email via the post editor, the button will also be centered. I spent a bunch of time trying to figure out why this is happening, but had no luck as of now. I will tackle this, as well as some of the other email designs in another PR. |
.editor-styles-wrapper .block-editor-block-list__layout .wp-block-button { | ||
margin-left: 0 !important; | ||
margin-right: 0 !important; | ||
} |
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.
Added these styles to resolve an issue where the button block was forced to be centered in the editor. This overrides the behavior in the editor, but actual emails will still see centered buttons for some reason. (Aside from our updated OTP default template)
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.
Trash the post (so that we can force our updated template to be used on next otp sign in attempt)
-
How will migration work for the sites out in the wild?
-
How will the template be updated in the event of site colors update?
Great questions @adekbadek
I think we want to avoid forcing sites to migrate since many publishers have likely put in some work on updating these emails on their end. My thought was to add an option to reset templates for those that would like to use our new designs. We might also want to add a similar reset link to each email template next to the edit link in the Transactional Email Content section: That said, I was hoping to focus this PR strictly on the design portion of this and tackle these issues in a follow up PR. Happy to add this in here as well if you prefer though!
This one is a bit tricky for publishers that have spent time customizing their email templates. Again, I'd avoid forcing these publishers to fall back to our designs on color update, and leave it up to them to "reset" via the links mentioned above, OR manually update their custom emails on their end. I'm definitely open to other suggestions for both of these questions too if you have any thoughts! |
I agree. But publishers who had not touched the templates should have them updated automatically.
As a publisher, I'd expect brand colors to be updated in the emails if they are updated for the site, even if email copy has changes. |
Sure! I'm happy to address this in a follow up PR, unless you feel it needs to be implemented here. My thought was to keep this PR and some follow-ups focused on the email redesign and handle the reset/update functionality separately. |
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.
Approving pending one small change requested and keeping in mind that the migration strategy should be addressed (as discussed above) before merging the epic branch.
includes/util.php
Outdated
*/ | ||
function newspack_get_theme_colors() { | ||
$primary_color = get_theme_mod( 'primary_color_hex', '#3366ff' ); | ||
$secondary_color = get_theme_mod( 'secondary_color_hex', '#f0f0f0' ); |
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.
Newspack Theme functions should be used (like here).
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.
Nice! I've fixed this in cc2f298
1a81695
to
9531069
Compare
9531069
to
cc2f298
Compare
🎉 This PR is included in version 3.6.0-epic-ras-acc.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
As a heads up @adekbadek, I've got some additional PRs up to tackle:
These three PRs should collectively address migration concerns. |
🎉 This PR is included in version 3.9.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.1.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
PR 1 of 4
This PR updates the OTP signin email with:
Screen.Recording.2024-04-04.at.15.01.16.mov
Note: Dynamic text color and FAQ sections added in #3033 and #3034
Note: I am only updating the OTP email for now. Once all of the kinks are worked out it should be pretty simple to update the others.
How to test the changes in this Pull Request:
Other information: