-
Notifications
You must be signed in to change notification settings - Fork 49
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 newsletters palette on theme primary color updates #3083
feat(ras-acc): update newsletters palette on theme primary color updates #3083
Conversation
d4a8b22
to
725786b
Compare
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.
After updating the theme color, the primary email color is visible when editing the email, but not in the sent email HTML.
includes/emails/class-emails.php
Outdated
@@ -488,6 +488,19 @@ public static function get_email_config_by_type( $type ) { | |||
/** Only attempt to create the email post if wp-includes/pluggable.php is loaded. */ | |||
return false; | |||
} else { | |||
// Make sure newsletters color palette is updated with latest theme colors. | |||
if ( self::supports_emails() ) { |
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.
Despite this check, the test are failing.
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.
Ah. I think this is because the Plugin_Manager
downloads the latest release for newsletters, which does not yet have the required method:
'Download' => 'https://github.com/Automattic/newspack-newsletters/releases/latest/download/newspack-newsletters.zip', |
I guess we can hold off on moving forward with this one until the next newsletters release.
Thanks for testing this @adekbadek!
That's strange. I am seeing the colors change in both the editor as well as the email: Screen.Recording.2024-05-07.at.15.03.20.movAre you resetting the email template once again after changing the primary/secondary colors by any chance? |
No. I tested by requesting the OTP as a reader, but when testing with the "Testing" sidebar panel (as on the video), the effect is the same. I've inspected the FWIW, I've found that on updating the theme color in the Site Design wizard, the first condition in the |
cd97ff6
to
132d897
Compare
Ah. We actually update the newsletters color palette in two places in this PR. The first is where you pointed out here, where we need to be sure the palette is updated with the latest before we initially create our email templates (after a template is reset for example). The second is here: newspack-plugin/includes/emails/class-emails.php Lines 656 to 665 in 9932035
This triggers whenever the sites primary or secondary color are updated. That said, I spoke with @dkoo about this and he pointed out the issue is actually related to a limitation with the mjml library in which newsletter HTML meta won't actually update unless the editor is open and the send/test button is clicked. Luckily, he was able to get a workaround for this added in 7463b3c So this should be working as expected now, regardless of how you trigger the email after updating site color. |
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.
Just noticed that maybe_update_email_templates
will only be called if no child theme is used (due to get_template()
use in the hook). The hook won't be called if a child theme is set, and so the email templates won't get updated.
Good catch @adekbadek! I've fixed in 96aaf98 |
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.
Child theme is now handled, but noticed a different issue 😬
includes/emails/class-emails.php
Outdated
'primary' => $theme_colors['primary_color'], | ||
'secondary' => $theme_colors['secondary_color'], | ||
'primary-text' => $theme_colors['primary_text_color'], | ||
'secondary-text' => $theme_colors['secondary_text_color'], |
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_get_theme_colors
does not return primary_text_color
and secondary_text_color
keys:
newspack-plugin/includes/util.php
Lines 549 to 552 in 725786b
return [ | |
'primary_color' => $primary_color, | |
'primary_text_color' => newspack_get_color_contrast( $primary_color ), | |
]; |
So it should be:
'primary-text' => newspack_get_color_contrast( $theme_colors['primary_color'] ),
'secondary-text' => newspack_get_color_contrast( $theme_colors['secondary_color'] ),
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.
Ah! That's right. We removed the secondary color from the email templates, so now this should only reference primary_color
and primary_text_color
. I've updated in e898500.
Thanks for the keen eye!
96aaf98
to
e898500
Compare
🎉 This PR is included in version 3.9.0-epic-ras-acc.4 🎉 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:
Part of https://app.asana.com/0/1206943664367847/1205668937699542/f
This PR is meant to be reviewed alongside these newsletters changes: Automattic/newspack-newsletters#1487
These changes ensure we are updating the newsletter color palette option with the latest primary and secondary theme colors before creating transactional emails from templates OR before triggering an update to email HTML when updating a sites primary or secondary newspack theme colors.
How to test the changes in this Pull Request:
Other information: