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: add brand_color variable for the email templates #33421

Conversation

bydawen
Copy link
Contributor

@bydawen bydawen commented Oct 5, 2023

Added a color variable to simplify branding of email templates used on different platforms. Also added a variable for the maximum height of the email logo image.

The main purpose of adding these variables is to be able to control the color used on buttons, links, etc. in emails. To avoid making these changes in many files, it can now be done by changing the value of one variable in the base_body template.

For example, as in the screenshot below, the size of the logo, the color of the button and links affected by brand_color variable which is setted here with the logo height.
{% with brand_color="#1b511a" logo_max_height="65px" %}

Similar PR is opened to the open-release/palm branch:
#33420

Screenshot 2023-10-05 at 17 45 15

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 5, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 5, 2023

Thanks for the pull request, @bydawen! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@bydawen
Copy link
Contributor Author

bydawen commented Oct 5, 2023

Hi! I'm contributing on behalf of Raccoongang

@bydawen bydawen force-pushed the feat/master/email-template-color-branding branch from d4cd264 to 4dd9300 Compare October 5, 2023 16:21
@bydawen bydawen changed the title feat: add brand_color variable for the email templates [WIP - DO NOT REVIEW] feat: add brand_color variable for the email templates Oct 6, 2023
@bydawen bydawen changed the title [WIP - DO NOT REVIEW] feat: add brand_color variable for the email templates feat: add brand_color variable for the email templates Oct 11, 2023
@dyudyunov dyudyunov force-pushed the feat/master/email-template-color-branding branch from 07a365a to 7e8021c Compare October 11, 2023 08:14
@mphilbrick211
Copy link

Hi @bydawen and @dyudyunov! Is this ready for review?

@bydawen
Copy link
Contributor Author

bydawen commented Oct 27, 2023

Hi @bydawen and @dyudyunov! Is this ready for review?

@mphilbrick211 hello, yes it's ready for review, thank you)

@mphilbrick211
Copy link

Hi @openedx/fed-bom! Is this something you could review / merge for us?

@mphilbrick211 mphilbrick211 added the backport PR backports a change from main to a named release. label Oct 27, 2023
@mphilbrick211
Copy link

Hi @openedx/fed-bom! I'm not sure if you are the correct squad, but if so, could someone please review and merge this for us? Thanks!

@bydawen bydawen force-pushed the feat/master/email-template-color-branding branch from bd1619e to c920000 Compare November 16, 2023 12:04
@bydawen bydawen force-pushed the feat/master/email-template-color-branding branch from c920000 to 75b1eb9 Compare November 16, 2023 12:31
@mphilbrick211
Copy link

Hi @BilalQamar95! Are you able to merge this along with the backport?

@mphilbrick211 mphilbrick211 removed the backport PR backports a change from main to a named release. label Nov 28, 2023
@BilalQamar95
Copy link
Contributor

Hi @mphilbrick211, we do have this in sight and will be merging this along with backport soon. Thank you

@GlugovGrGlib
Copy link
Member

GlugovGrGlib commented Dec 27, 2023

@BilalQamar95 Hello, are there any estimates regarding the merge date for this and Quince backport? Thanks

@BilalQamar95
Copy link
Contributor

@BilalQamar95 Hello, are there any estimates regarding the merge date for this and Quince backport? Thanks

@GlugovGrGlib We plan to merge this within this week

Comment on lines +409 to +411
'your édX account from',
old_email,
new_email,
Copy link
Contributor

Choose a reason for hiding this comment

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

why have you updated these formatted strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

because the inline styling was applied to emails

Here is how it used in the template and actual changes for the email variables

@mphilbrick211
Copy link

Hi @abdullahwaheed! Are you able to merge this for us?

@abdullahwaheed
Copy link
Contributor

Hi @abdullahwaheed! Are you able to merge this for us?

i'll merge it tomorrow

@abdullahwaheed abdullahwaheed merged commit 4ec70eb into openedx:master Jan 23, 2024
64 checks passed
@openedx-webhooks
Copy link

@bydawen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

justinhynes added a commit that referenced this pull request Jan 24, 2024
This reverts commit 4ec70eb.

This commit introduced a new setting (`brand_color`) that does not appear to be set and is causing issues with account deletion and other parts of the courseware.

Reverting until we can understand the change better.
justinhynes added a commit that referenced this pull request Jan 24, 2024
revert: add brand_color variable for the email templates (#33421)"
@cmltaWt0 cmltaWt0 added the product review PR requires product review before merging label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants