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

Updated OrderReceiptStyle.ss for Email CSS Resets #441

Merged
merged 1 commit into from Mar 23, 2016
Merged

Updated OrderReceiptStyle.ss for Email CSS Resets #441

merged 1 commit into from Mar 23, 2016

Conversation

AntonyThorpe
Copy link
Contributor

Uncommented the global resets in OrderReceiptStyle.ss and provided some base styles as recommended by seanpowell/Email-Boilerplate. Also added a table#container wrapper to provide left/right padding in emails. Thanks.

Added container table around table#Content in all email templates
Uncommented CSS in OrderReceiptStyle.ss and provided Global Reset CSS for email (this contains the base left and right padding of 20px)
Added a title line height to order.css
@markguinn
Copy link

@bummzack do you have any thoughts on this? I know we'd talked some about reworking these emails based on MailChimp's templates or Foundation for Emails. Do you think this serves the same purpose and makes that task unnecessary? It looks to me like it would improve the quality of the default emails.

@bummzack
Copy link
Collaborator

Yeah, this looks like a much better Email template. The problem is, that this PR seems to remove a lot of the inlined styles and relies solely on the inclusion of the external CSS?
Or how did you add the order-styles to the Email @AntonyThorpe ?

Some of the points in #414 are still valid, although possible outside of the scope of this PR alone. I don't like having CSS in .ss files either.

Ideally we'd have inlined CSS from a CSS file, merged into the HTML via emogrifier. Could all of this be implemented in your Email helper module? Maybe even add inline-images as a feature?

@markguinn
Copy link

True enough. It looks like PHPMailer (which my email-helpers module uses) can inline the images for you (http://stackoverflow.com/questions/1851728/how-to-embed-images-in-html-email) so one option would be to include this and add my module to the "suggested" list.

We could then require it in the 2.0 branch and swap out Email for StyledHtmlEmail in the order notifier. OR we could just integrate Emogrifier directly as it's a pretty simple affair.

@bummzack
Copy link
Collaborator

I think we should require your module for 2.0. I think everybody wants to send properly formatted Emails that look good in most clients, unless they have a separate solution for Emails?

I'd keep things as they are handled currently for 1.x, so that your module is optional there. We can also discuss this matter with the other core-team members first.

@markguinn
Copy link

Great. Did you see my note that I need to re-schedule the meeting for tonight?

@bummzack
Copy link
Collaborator

Oh, no I didn't. Good to know. Where did you post that note?

@markguinn
Copy link

On the Doodle poll. I've just sent you guys an email about it.

@AntonyThorpe
Copy link
Contributor Author

@bummzack the CSS in OrderReceiptStyle.ss is currently commented out so is not used. You are right Silvershop currently relies on an external file, order.css (via order.ss), to provide all the email styling. We could place the email resets in an external file as well. Would this be a better approach?

Note: the comment element in OrderReceiptStyle.ss currently breaks the emogrifier process of the Silverstripe Email Helpers module.

@markguinn
Copy link

I just created a PR against 2.0 that requires the email-helpers module. Now that I'm thinking about it, that could probably go in feature release as it doesn't really break anything. What do you guys think? Should I rebase that PR against master and it can go into 1.3 along with this?

@bummzack
Copy link
Collaborator

@markguinn Sounds good to me.

@markguinn
Copy link

Ok. I'm going to go ahead and merge this and rebase my work on top of this.

@markguinn markguinn merged commit 62e6d77 into silvershop:master Mar 23, 2016
@markguinn
Copy link

@AntonyThorpe - I may have merged this prematurely. The default emails now look a lot less nice than they used too.

https://www.dropbox.com/s/7tcfefmnd7j3n70/Screenshot%202016-03-23%2015.02.04.png?dl=0

vs.

https://www.dropbox.com/s/29gqxookc1qhwm1/Screenshot%202016-03-23%2015.03.46.png?dl=0

What do you think about adding back some of the old styling on top of these resets? (I'm happy to do the work, just curious if there was more to your master plan). Is this about how your emails are looking?

@AntonyThorpe
Copy link
Contributor Author

@markguinn Sorry, that is quite different. My emails are coming through like your second link, fully styled. The plan is to get the EmogrifiedSmtpMailer class of Silverstripe Email Helpers working without the comment element in OrderReceiptStyle.ss causing errors. Then I thought a left/right margin was needed and some base email resets. Will take a closer look and get back to you.

@markguinn
Copy link

I had a few merge conflicts in pulling this into 2.0 and merging it with my emogrifier work. I might have bastardized your changes a bit.

@AntonyThorpe
Copy link
Contributor Author

@markguinn Have worked it out. I thought that the commented out styles in OrderReceiptStyle.ss were not used in emails, but they are. Also, order.css is not used for emails (my comments above are incorrect). Will send through a pull request to add back the reset styles.

@AntonyThorpe AntonyThorpe deleted the OrderReceiptStyle branch March 23, 2016 22:00
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.

None yet

3 participants