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

Static versioning and styles minification break email fonts styles #8241 #10638

Merged
merged 2 commits into from
Sep 4, 2017

Conversation

xmav
Copy link
Contributor

@xmav xmav commented Aug 23, 2017

Static versioning and styles minification break email fonts styles #8241

Description

With old version of pelago/emogrifier minified css is not applied to html document(but works fine with normal css). This leads to broken styles in email notifications

Fixed Issues (if relevant)

  1. Static versioning and styles minification break email fonts styles #8241: Static versioning and styles minification break email fonts styles

Manual testing scenarios

  1. Create an order for user with valid email
  2. Check order details email
    Actual result:
    Minified inline css is not applied (added by {{inlinecss file="css/email-inline.css"}}) to html document (or applied partially)

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)



 - update pelago/emogrifier version to the latest
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 23, 2017

CLA assistant check
All committers have signed the CLA.

@miguelbalparda miguelbalparda self-assigned this Aug 23, 2017
@miguelbalparda miguelbalparda added this to the August 2017 milestone Aug 23, 2017
@miguelbalparda
Copy link
Contributor

This seems to fix only the minification issue but I would still like to process this since the version of this extension is really outdated. Do you foresee any issues in any other parts of the app with this change? Adding @Igloczek to the loop for extra comments.

@xmav
Copy link
Contributor Author

xmav commented Aug 23, 2017

@miguelbalparda No I don't foresee problems with this change

@xmav
Copy link
Contributor Author

xmav commented Aug 23, 2017

#4737
#6946
#9327

@xmav
Copy link
Contributor Author

xmav commented Aug 24, 2017

@miguelbalparda Could you please re-run Travis build? Looks like random failure

@miguelbalparda
Copy link
Contributor

Restarted

@miguelbalparda
Copy link
Contributor

I'll wait for a reply in #4737 before accepting this PR.

@davidalger davidalger self-assigned this Sep 1, 2017
@davidalger davidalger self-requested a review September 1, 2017 15:19
Copy link
Member

@davidalger davidalger left a comment

Choose a reason for hiding this comment

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

@miguelbalparda This looks good to me. I see no reason to hold off on merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants