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.

Sign up for GitHub

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

Jump to bottom

feat: Support for AMP HTML Email #945

Merged
merged 14 commits into from
Jan 8, 2021
Merged

feat: Support for AMP HTML Email #945

merged 14 commits into from
Jan 8, 2021

Conversation

modernwarfareuplink
Copy link
Contributor

@modernwarfareuplink modernwarfareuplink commented Oct 29, 2020 •
edited
Loading

I acknowledge that my contributions will be made under the same open-source license that the open-source project is provided under

Adds support for sending AMP HTML email

  • Added third mime type -> text/x-amp-html
  • Send AMP HTML email just like normal html/plain email

Resolves

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 29, 2020
@modernwarfareuplink modernwarfareuplink mentioned this pull request Oct 29, 2020
Closed
@modernwarfareuplink
Copy link
Contributor Author

@thinkingserious need your superpowers here !!! . Please check and let me know about the PR. Thanks

@modernwarfareuplink
Copy link
Contributor Author

modernwarfareuplink commented Nov 13, 2020 •
edited
Loading

@thinkingserious gentle reminder. Please let me know if you need any help regarding review. Thanks

thinkingserious
thinkingserious requested changes Nov 16, 2020
View reviewed changes
Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Thank you!

use_cases/sending_amp_html_content.py Outdated Show resolved Hide resolved
use_cases/sending_amp_html_content.py Outdated Show resolved Hide resolved
use_cases/sending_amp_html_content.py Outdated Show resolved Hide resolved
sendgrid/helpers/mail/mail.py Show resolved Hide resolved
@modernwarfareuplink
Copy link
Contributor Author

Thanks for the review @thinkingserious. Will update in 2 days, caught up with something. Will let you know, once the changes are done.

@thinkingserious
Copy link
Contributor

That sounds good @modernwarfareuplink, thank you!

@thinkingserious thinkingserious added status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels Dec 3, 2020
thinkingserious
thinkingserious reviewed Dec 31, 2020
View reviewed changes
Copy link
Contributor

@thinkingserious thinkingserious left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thank you!

Could you please update the tests here to finish this one off? https://github.com/sendgrid/sendgrid-python/blob/HEAD/test/test_mail_helpers.py

Thanks again!

@modernwarfareuplink
Copy link
Contributor Author

Thanks for the review. I am really sorry that it took this long and no updates from my side. Will finish adding the tests by morning (in around 12 hours) and let you know.
Thank you and happy new year @thinkingserious

@modernwarfareuplink
Copy link
Contributor Author

@thinkingserious have resolved your comments, made few changes around the ordering and added appropriate tests. Also found that the build was failing due to copyrights year and fixed it too. Please review and let me know if you need any additional details. Thank you

@modernwarfareuplink
Copy link
Contributor Author

@thinkingserious gentle reminder for review. Please let me know if you have any issues. Thank you

JenniferMah
JenniferMah suggested changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@JenniferMah JenniferMah left a comment

Choose a reason for hiding this comment

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

Thanks for updating the tests! I've just requested a few changes but over all this looks good to me!

test/test_mail_helpers.py Outdated Show resolved Hide resolved
test/test_mail_helpers.py Outdated Show resolved Hide resolved
@modernwarfareuplink
Copy link
Contributor Author

Have updated the minor comment @JenniferMah . Please do have a look once. Thank you

@modernwarfareuplink
Copy link
Contributor Author

@thinkingserious need your superpowers for review and merging the changes 🙏

@JenniferMah JenniferMah merged commit 39844d6 into sendgrid:main Jan 8, 2021
@modernwarfareuplink
Copy link
Contributor Author

Thank you @JenniferMah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JenniferMah JenniferMah JenniferMah approved these changes

@thinkingserious thinkingserious Awaiting requested review from thinkingserious

Assignees
No one assigned
Labels
status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for text/x-amp-html mime type It doesn't seem to support third mime type AMP
3 participants
@modernwarfareuplink @thinkingserious @JenniferMah