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

Fix custom styles for emails in preview and in sent messages #7595

Merged
merged 11 commits into from
Jul 31, 2024

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Apr 22, 2024

Resolves #7459

Some styling that comes by default with WordPress couldn't be applied to emails.

The main restriction comes from Gmail that removes unsupported styling from emails in their web-app. For this reason, we disable some settings in this PR.

We address the issue with misstyling of elements due to lack or conflict of styles.

Proposed Changes

  • Load global styles for inline CSS processor.
  • Disable default color palettes.
  • Disable alignment for buttons and images.
  • Remove some default from editor settings that affect the appearance of the email in the editor only.

Testing Instructions

For testing real messages, use Gmail webapp as it has more restrictive rules for CSS comparing to desktop applications.

  1. Go to Sensei LMS > Settings > Emails.
  2. Enable the Course Completed email and then edit it.
  3. Change colors for buttons, paragraphs.
  4. Change text alignment.
  5. Add an image.
  6. Make sure you don't have controls to change alignment of the image or buttons.
  7. Save and preview. Make sure you see your changes in the preview.
  8. Take a look at the button and make sure it has the same padding as in the editor.
  9. Complete the course as a student.
  10. Check your mailbox and make sure that the Course Completed email reflects your changes.

Screenshots

Real email

(You can see that Sensei logo and the image in the body are not displayed — that's because they refer to my local website.)

Gmail web-app

CleanShot 2024-06-02 at 15 29 49@2x

Mail App

CleanShot 2024-06-02 at 15 30 24@2x

Editor

CleanShot 2024-06-02 at 15 27 31@2x

Preview

CleanShot 2024-06-02 at 15 27 44@2x

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@merkushin merkushin added this to the 4.24.0 milestone Apr 22, 2024
@merkushin merkushin requested a review from a team April 22, 2024 04:45
@merkushin merkushin self-assigned this Apr 22, 2024
Copy link

Test the previous changes of this PR with WordPress Playground.

* @param array $placeholders The placeholders.
*
* @return string
*/
private function replace_placeholders( string $string, array $placeholders ): string {
private function replace_placeholders( string $content, array $placeholders ): string {
Copy link
Member Author

Choose a reason for hiding this comment

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

PHPCS was complaining for the variable name.

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

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

For some reason it's not working for me -

Editor:

Screenshot 2024-04-23 at 4 30 02 PM

Gmail:

Screenshot 2024-04-23 at 4 31 42 PM

Temp mail client:

Screenshot 2024-04-23 at 4 32 20 PM

@merkushin
Copy link
Member Author

merkushin commented Apr 23, 2024

@Imran92 Thanks for testing!

Could you tell me what are the versions of WordPress and Gutenberg (if installed), and what theme do you use on the test website?

@m1r0
Copy link
Member

m1r0 commented Apr 23, 2024

I've tested it and the colors worked, but the alignment didn't. The result was the same with and without the Gutenberg plugin.

@Imran92
Copy link
Contributor

Imran92 commented Apr 24, 2024

Could you tell me what are the versions of WordPress and Gutenberg (if installed), and what theme do you use on the test website?

Hi @merkushin 👋 I've shared the creds of a JN site where I reproduced the issue in DM. Hope it'll help! Thanks! Locally I didn't have GB installed and WP version was 6.5.2

@merkushin
Copy link
Member Author

Thanks Imran!

Ah, yes, the same email in Gmail and in the Mail app look so differently.

CleanShot 2024-04-23 at 23 04 06@2x

CleanShot 2024-04-23 at 23 03 52@2x

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link

Test the previous changes of this PR with WordPress Playground.

@donnapep donnapep removed this from the 4.24.0 milestone Apr 25, 2024
@merkushin merkushin added this to the 5.0.0 milestone May 9, 2024
Copy link

github-actions bot commented May 9, 2024

Test the previous changes of this PR with WordPress Playground.

@merkushin
Copy link
Member Author

merkushin commented May 9, 2024

As Imran suggested, I checked that when we don't use default colors and the picker instead we get colors as the HEX value, so I disabled default colors in the editor settings.
2a2ec4e

With alignment is a bit harder. I added classes needed we missed in CSS, now it works in Preview and in Mail app. But Gmail webapp "doesn't work". I found they just don't support some CSS properties (for example, justify-content and align-items—both are used to align buttons).
b8a6aab
And it is impossible to disable in the editor settings.

I also added styles to make text alignment work.

I still see other issues with styles in emails:

  • The size of the button is different in Editor vs Preview/Real Message.
  • Image alignment doesn't work. I tried to fix it quickly by bringing needed classes, but it still misses the width property (not sure if it is the only cause), so it doesn't work.

And I suppose we can find even more tiny issues like those ones. I think we need to add a warning to our documentation that not all styles are supported in emails. And maybe list the ones we know already.

Copy link

github-actions bot commented May 9, 2024

Test the previous changes of this PR with WordPress Playground.

@donnapep
Copy link
Collaborator

@merkushin @Imran92 What's the status of this one? Pending another review or something else?

@Imran92
Copy link
Contributor

Imran92 commented May 30, 2024

@merkushin @Imran92 What's the status of this one? Pending another review or something else?

I'll take a look at the last changes Dmitry made today!

@Imran92
Copy link
Contributor

Imran92 commented May 30, 2024

As Imran suggested, I checked that when we don't use default colors and the picker instead we get colors as the HEX value, so I disabled default colors in the editor settings.

Glad it helped!

Can you please update the "Proposed Changes" and the "Testing Instruction" of the PR body to point out what changes are made, what to test and what are expected not to work yet?

Thanks!

@Imran92
Copy link
Contributor

Imran92 commented May 30, 2024

And it is impossible to disable in the editor settings.

I wrote the following code to remove alignment settings from a block in the editor -

wp.hooks.addFilter(
    'blocks.registerBlockType',
    'imran/imranFilterName',
    removeJustificationAndAlignmentsFromButtons
);

function removeJustificationAndAlignmentsFromButtons(settings, name) {
    if (name !== 'core/buttons') {
        return settings;
    }

    return lodash.assign({}, settings, {
        supports: lodash.assign({}, settings.supports, {
            layout: false,
        }),
    });
}

Can you try and see if it works for you?

Without the script-

image

With the script-
image

This is just an example, it should be technically possible to remove all types of support this way. But there can be other issues which I haven't faced here but you did. Also, this code is just a quick one, proper syntax should be used for production.

Overall, most settings are working now. I've seen some issues as well.

  • Button has padding setting but it doesn't work
  • Some color changes do not work
Screen.Recording.2024-05-30.at.5.13.58.PM.mov

@merkushin merkushin marked this pull request as draft May 30, 2024 18:42
Copy link

github-actions bot commented Jun 2, 2024

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 0c6a548 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
blocks/email-editor.js 1.05 kB +90 B ( +9.42% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

Copy link

github-actions bot commented Jun 2, 2024

Test the previous changes of this PR with WordPress Playground.

@merkushin
Copy link
Member Author

Thanks @Imran92! Applied your suggestion to remove alignment for buttons and images.

I updated the PR description. The PR is ready for review.

Here are the details of the most recent changes:

With padding, there are the following styles that affect it:

  • we have block-library/styles.css with calc for padding
  • we have editor styles that use CSS variables for padding

Neither of them are loaded in the preview or for the actual email, and, basically, loading them doesn't make much sense if our goal is to look good in the Gmail web-app.

We also have our custom padding styles. By making the rule there !important we can make the appearance consistent. However, it makes impossible to apply custom padding in the editor.

While inspecting the editor settings, I found there is an array of styles. Setting it the an empty array removes a lot of unexpected (for us) styles and makes it possible to use padding.

It also makes alignment not working even in the editor. Which is good for us, I think, because there is a consistency between the editor and the preview. As I said in the beginning of the comment, the alignment feature was disabled anyway.

Also, I found the rule that defined the text color (that didn't work in Imran's test). I removed it: I didn't find bad consequences of that, but I'm not 100% sure why it was added in first place. I can imagine it was a desire to follow the proposed design for the Email project.

@merkushin merkushin marked this pull request as ready for review June 2, 2024 21:51
@m1r0 m1r0 modified the milestones: 5.0.0, 4.24.1 Jun 4, 2024
@donnapep donnapep modified the milestones: 4.24.1, 4.24.2 Jun 13, 2024
m1r0
m1r0 previously approved these changes Jul 25, 2024
Copy link
Member

@m1r0 m1r0 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 working on this Dmitry. It works well in my opinion.

I've noticed that when you have a very wide image, the height is not good:

Editor Gmail/Preview
image image

You can see that in Gmail/Preview the pickle's height is bigger. Adding (max-)height: 100% fixes it.

The above is not a blocker and can be fixed in a separate PR.

Copy link

Test the previous changes of this PR with WordPress Playground.

@merkushin
Copy link
Member Author

merkushin commented Jul 25, 2024

Thanks, @m1r0! I updated the PR, added 100% height for <img/> inside .wp-image-block.

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Sorry for the back and forth, but I've noticed that the image is missing a box-sizing: border-box. The issue is visible when you add a border to the image - it extends past the container width.

Without box-sizing: border-box With box-sizing: border-box
image image

assets/css/email-notifications/email-style.scss Outdated Show resolved Hide resolved
Copy link

Test the previous changes of this PR with WordPress Playground.

@merkushin
Copy link
Member Author

merkushin commented Jul 28, 2024

Thanks @m1r0! I updated the PR. Could you look at it again? 😅

CleanShot 2024-07-28 at 13 28 35@2x

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the updates. 🙏

Copy link

Test the previous changes of this PR with WordPress Playground.

@merkushin merkushin merged commit 53dbffa into trunk Jul 31, 2024
23 checks passed
@merkushin merkushin deleted the fix/email-styles branch July 31, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Style changes made in email editor are not reflected in the preview or in sent emails
4 participants