Skip to content
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): dynamically set otp email text color #3033

Merged

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Apr 4, 2024

All Submissions:

Changes proposed in this Pull Request:

PR 2 of 4

This PR adds onto the work in #3032 by adjusting email text, button, and social icon color for the default email template to either white or black depending on background color:

Screenshot 2024-04-04 at 17 35 31

Note: The changes only apply to the one time password email for the time being. Once that email is fully updated, I will get a PR up to update the remaining templates.

How to test the changes in this Pull Request:

  1. Go to the WP Customizer and set a dark custom primary and secondary color
  2. With these changes checked out, go to Newspack > Engagment > Show Advanced Settings and select the one-time password email under Transactional Email Content section
  3. Trash the post (so that we can force our updated template to be used on next otp sign in attempt)
  4. As a logged out user, select the sign in link and attempt to login via email to trigger an otp email
  5. Check the otp email and confirm all of the text is properly contrasting
  6. Repeat the above steps with light custom colors in customizer

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@chickenn00dle chickenn00dle marked this pull request as ready for review April 4, 2024 21:40
@chickenn00dle chickenn00dle requested a review from a team as a code owner April 4, 2024 21:40
@chickenn00dle chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 4, 2024
@chickenn00dle chickenn00dle force-pushed the feat/update-transaction-email-colors branch from 9531069 to cc2f298 Compare April 9, 2024 14:48
@chickenn00dle chickenn00dle changed the base branch from feat/update-transaction-email-colors to epic/ras-acc April 9, 2024 15:10
@chickenn00dle chickenn00dle changed the base branch from epic/ras-acc to feat/update-transaction-email-colors April 9, 2024 15:10
Base automatically changed from feat/update-transaction-email-colors to epic/ras-acc April 9, 2024 15:11
@chickenn00dle chickenn00dle force-pushed the feat/update-transaction-emails-dynamic-colors branch from 0bce5a5 to 2b4f3d6 Compare April 9, 2024 15:32
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues with the template file:

  1. the $email_html is not updated along with the $post_content changes
  2. there's a hard-coded OTP code with 895671 value (instead of a placeholder)

includes/templates/reader-activation-emails/otp.php Outdated Show resolved Hide resolved
includes/templates/reader-activation-emails/otp.php Outdated Show resolved Hide resolved
@chickenn00dle chickenn00dle force-pushed the feat/update-transaction-emails-dynamic-colors branch from 0989b8a to e4c8473 Compare April 16, 2024 22:20
@chickenn00dle
Copy link
Contributor Author

Thanks for the review @adekbadek!

the $email_html is not updated along with the $post_content changes

I've updated the email html and did some formatting to hopefully make parsing the email a bit easier.

there's a hard-coded OPT code with 895671 value (instead of a placeholder)

Good catch. Fixed in 6510cc3

I also made some additional changes to how we set colors in the default block in e574d6c and e4c8473. I realized the previous method would not allow the email template to update when theme colors were changed since the block colors were being explicitly set as hex values.

Unfortunately, it looks like we will not be able to use a dynamic approach for social icons without some changes in the newsletter plugin first since newsletters pretty strictly decides on a colorsheme for social icon images when generating the email html markup. (at least I wasn't able to find a straightforward way without having to make changes in newsletters)

So I've reverted the icons to use the default layout and colorscheme for the time being. I will try to address them in a follow-up PR once I've worked out all of the other design bits for emails:

Screenshot 2024-04-16 at 18 30 13

@chickenn00dle chickenn00dle requested a review from adekbadek April 16, 2024 22:32
includes/util.php Outdated Show resolved Hide resolved
@chickenn00dle chickenn00dle requested a review from adekbadek April 17, 2024 13:57
@github-actions github-actions bot added the [Status] Approved The pull request has been reviewed and is ready to merge label Apr 18, 2024
@github-actions github-actions bot removed the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 18, 2024
@chickenn00dle chickenn00dle merged commit 58424b9 into epic/ras-acc Apr 18, 2024
9 checks passed
@chickenn00dle chickenn00dle deleted the feat/update-transaction-emails-dynamic-colors branch April 18, 2024 16:12
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.6.0-epic-ras-acc.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.9.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.1.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @epic/ras-acc [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants