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

Update emogrifier #2809

Merged
merged 12 commits into from
Apr 25, 2024
Merged

Update emogrifier #2809

merged 12 commits into from
Apr 25, 2024

Conversation

mikeyarce
Copy link
Member

@mikeyarce mikeyarce commented Apr 20, 2024

Fixes #2755

Changes Proposed in this Pull Request

  • Update to use new version of Emogrifier
  • Load Emogrifier through a composer dependency
  • New plugin build so that we can include the composer dependency

Testing Instructions

  • Check out PR
  • Run npm run build
  • Make sure that the new plugin build matches the old plugin build. You can run npm run build:assets && npm run archive which is the previous build and check folders to make sure it's what we expect. It should be identical except for the tests are gone from the usage-tracking folder, and now we do include the vendor folder for emogrifier.
    Here is my folder comparison including hidden files:
    gAxNYC3MaLQmZ3igYlIJ4OxbwHPVpEbvzaubHnA8.jpg
  • Now use this plugin on a site that can send emails
  • Add a few alerts
  • Send the alerts to a gmail account and make sure the top and bottom margin is there!

Example of margin:
UkMfLY0Xzpw2hoG0xZdWcm1JMJ85f47VCTlQ9MHX.jpg

Release Notes

  • Fix: Email notifications - top and bottom margin not displaying properly for some email clients.

Plugin build for 9215cf6
📦 Download plugin zip
▶️ Open in playground

@mikeyarce mikeyarce requested a review from a team April 20, 2024 20:54
@mikeyarce mikeyarce self-assigned this Apr 20, 2024
Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

Works well! Left a few comments

includes/class-wp-job-manager-email-notifications.php Outdated Show resolved Hide resolved
includes/class-wp-job-manager-email-notifications.php Outdated Show resolved Hide resolved
@@ -34,7 +34,7 @@
"zx": "^7.2.3"
},
"scripts": {
"build": "npm run build:assets && npm run archive",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we are going to replace the archive command? If this is the case, maybe it makes sense to remove the archive.exclude arguments from composer.json? Keep in mind although it is called exclude we do a ! so they are actually includes.

We should also remove the old archive command

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 9215cf6

@mikeyarce
Copy link
Member Author

Thanks @gikaragia - made the changes 😄

Copy link
Contributor

@gikaragia gikaragia left a comment

Choose a reason for hiding this comment

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

LGTM!

@yscik yscik merged commit 2224b9e into trunk Apr 25, 2024
11 checks passed
@yscik yscik deleted the update/emogrifier branch April 25, 2024 10:12
@yscik yscik added this to the 2.3.0 milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E-mail top and bottom margin missing
3 participants