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

Add new message preview field to campaign composer for mail client previews #654

Merged
merged 5 commits into from May 22, 2020

Conversation

samtuke
Copy link
Contributor

@samtuke samtuke commented May 11, 2020

Description

~All popular mobile mail clients include a preview of email content as well as the subject line. This is common functionality among other ESPs.

The text preview is currently hard to know using phpList unless you send a test message. Mail clients read the plaintext version of the message and take the first 35-90 chars for preview. In phpList the 'text' tab is optional, and often disabled. In such cases it's not possible to know the mail client preview before sending the campaign (when plaintext version is presumably generated at queue processing time).

Therefore this PR adds a message preview to the html campaign composer page (content tab). It has simple logic, using the existing backend functions for retrieving saved message content and turning into plaintext. It also removes newlines and other problematic characters. Via a new ajax file and call it loads the saved html campaign content and turns into plaintext, abbreviated to 90 chars. This is then shown to the admin in a non-editable input text field.

This implementation seems to work for my and hopefully general use cases. However it has some shortcomings:

  • Field order and size isn't ideal; might be better to move the new field below the campaign content area or elsewhere to prevent the top of the form being too cluttered
  • It loads the preview from the saved content, not from the html content textarea. That means you have to save your changes before the preview will reflect them. That could be easily changed (and would actually be a simpler approach than the current ajax call which loads content from database). However that approach would also be more limiting in that the preview would only work on the composer page (which it is currently on), and could not be moved to another tab, which may be desirable for UX reasons.
  • The whole 90 chars currently isn't visible in the input field without sidescrolling. That's partly by design as I didn't want to take up even more space on the page, but the text field could be replaced with a div or something else.
  • Placeholders are not replaced, leading to ugly square braces etc. It'd be better if these were parsed during preview generation and replaced with admin attributes (eg admin first name for [FIRST NAME]. However that would increase the workload of this PR so I avoided it; it'd be nice if others were to add that.
  • The 'Generate' link should probably be a button (UX people please improve as desired)
  • The 'Generate' link disappears after it's pressed -- this is partly by design (as you need to save changes before regenerating the preview makes sense (see above note on how the preview is retrieved from the backend), and partly becuase this is the default behaviour of phpList's ajax functions, or at least this implementation copied from lists/admin/actions/generatetext.php
  • Reference to a new help prompt 'generatetextpreview' is made, but I have not added it. I think it's more efficient to check the reaction to this PR before making that, as it will be a separate PR to the help repo which could be orphaned or delayed depending on this PR's fate.
  • Ditto 'Campaign preview' English string for translation.

Related Issue

None that I can find

Screenshots (if appropriate):

Peek 2020-05-11 15-04

@samtuke
Copy link
Contributor Author

samtuke commented May 11, 2020

CI errors 😢 :

Could not open connection: session not created: This version of ChromeDriver only supports Chrome version 80

As this issue is project-wide I'll leave it to others to resolve

@suelaP
Copy link
Member

suelaP commented May 11, 2020

@samtuke the problem is now fixed in master, if you rebase the PR should pass

@samtuke
Copy link
Contributor Author

samtuke commented May 11, 2020

@samtuke the problem is now fixed in master, if you rebase the PR should pass

Thanks; done

@suelaP
Copy link
Member

suelaP commented May 12, 2020

@samtuke I don't think the following bit is working properly:

It has simple logic, using the existing backend functions for retrieving saved message content and turning into plaintext.

If you paste your entire HTML campaign in the content tab using the "source" option and generate the preview you will end up with the styling properties in the preview.

generated-preview

text-version

@samtuke
Copy link
Contributor Author

samtuke commented May 12, 2020

Thanks for testing!

From memory, the backend logic is identical to the button which generates plain text on the text tab, so I dont immediately understand how it's possible that CSS or html could be included in the preview. However I'll debug it.

@bramley
Copy link
Contributor

bramley commented May 13, 2020

It should probably convert html to text on the complete message and then take the first 90 characters. Otherwise there is a possibility of having an incomplete html element.

I don't understand these lines. The comments don't match the code, and the second chunk is redundant.

// replace newlines with spaces
$previewText = str_replace("\r", "", $previewText);
$previewText = str_replace("\n", "", $previewText);
// replace escaped newlines
$previewText = preg_replace("/\n/", '\\n', $previewText);
$previewText = preg_replace("/\r/", '', $previewText); 

Do you have an example (a screenshot) of a mail client showing this? In yahoo webmail and gmail they show the "preheader", which is otherwise hidden text that is additional to the email body.
image
The bit 'new recordings ...' is not displayed when viewing the email.

An alternative is to provide an input field allowing the admin to enter a preheader (the Content Areas plugin does this).

If a preheader is not present then yahoo and gmail seem to show the first chunk of text. But in phplist some of that can come from the template, which this change doesn't appear to use.

Update
This is an email sent from phplist in the web interface to gmail. The preview text is taken from the html part of the email, not the plain text part. The "About Us ..." is in the phplist template not the phplist message content.

image

@samtuke
Copy link
Contributor Author

samtuke commented May 14, 2020

I don't understand these lines. The comments don't match the code, and the second chunk is redundant.

Good point. I copied them from generatetext.php. Now removed in b61fe44

An alternative is to provide an input field allowing the admin to enter a preheader (the Content Areas plugin does this).

I didn't realise that functionality was included there. Personally, as a user, I think preheader is important enough to be moved to core, at some stage (or the plugin to be shipped with core).

If a preheader is not present then yahoo and gmail seem to show the first chunk of text. But in phplist some of that can come from the template, which this change doesn't appear to use.

Ouch, great point. I guess the template must be combined with the html to catch that. Which is more complex...

This is an email sent from phplist in the web interface to gmail. The preview text is taken from the html part of the email, not the plain text part. The "About Us ..." is in the phplist template not the phplist message content.

Was there no content in that campaign apart from the "About us" footer then?

The implementation in this PR uses the message array element from loadMessageData(), which I believe is the HTML version of the campaign content, not the plaintext version. So, template aside, it should be generating the same plain text version of the content as Gmail uses.

@samtuke
Copy link
Contributor Author

samtuke commented May 14, 2020

It should probably convert html to text on the complete message and then take the first 90 characters. Otherwise there is a possibility of having an incomplete html element.

Fixed in 750721d

@bramley
Copy link
Contributor

bramley commented May 14, 2020

@samtuke can you rebase your code on the latest master to avoid the irrelevant commits that bring your code up to the master?

@samtuke
Copy link
Contributor Author

samtuke commented May 14, 2020

@samtuke can you rebase your code on the latest master to avoid the irrelevant commits that bring your code up to the master?

It was already rebased, now force-pushed, should be clean

@samtuke
Copy link
Contributor Author

samtuke commented May 14, 2020

@bramley assuming that the template issue can be resolved, it seems this feature will always need to be used in combination with the content areas plugin's preheader field, in order to know the preview text for the widest amount of mail clients. That's inconvenient and potentially misleading for the user of either solution in the absence of the other, it seems to me.

Ideally users want a single place they can set the preview text, or at least see it, not two different ones. Not sure of the solution.

@michield
Copy link
Member

It's a great addition. It's quite common these days.

Things I've noticed:

  1. When you change tabs, the value doesn't save. All other fields save automatically.
  2. Placeholder can be cut off, which will mean they will stay verbatim.
    image
  3. Why is the text not editable once you've clicked "generate". That could solve issue 2.

@bramley
Copy link
Contributor

bramley commented May 15, 2020

Ideally users want a single place they can set the preview text, or at least see it, not two different ones.

I didn't mean to suggest that people use the Content Areas plugin, as I think it is a bit too difficult to use, but having a preheader field lets the admin specify exactly what additional text appears.

I don't use enough email clients to know which will not use preheader, in which case this addition of preview is useful.

@samtuke
Copy link
Contributor Author

samtuke commented May 18, 2020

The implementation in this PR uses the message array element from loadMessageData(), which I believe is the HTML version of the campaign content, not the plaintext version. So, template aside, it should be generating the same plain text version of the content as Gmail uses.

With 503bf10 the plaintext campaign text is used in the preview, if it exists, instead of new text generated from the HTML as before.

However for users which have the text tab disabled, which is presumably the majority, their only option to improve the preview with this PR's implementation is to change the start of their HTML content (which is then used as the basis for generated plaintext when messages are generated).

Inserting (and editing) a "preheader" (by which I mean invisible text at the top of the HTML content hidden using style rules) could be added to this PR to give users the ability to change the preview without changing their campaign content - eg as two separate buttons next to the existing 'Generate'.

However doing that would 1. duplicate functionality of the content areas plugin, and 2. Cause problems for plaintext users, as the preheader text would unexpectedly be visible for plaintext campaign readers (and users would not know this while editing thanks to missing 'text' tab). Thoughts @bramley ?

@suelaP
Copy link
Member

suelaP commented May 21, 2020

However doing that would 1. duplicate functionality of the content areas plugin, and 2. Cause problems for plaintext users, as the preheader text would unexpectedly be visible for plaintext campaign readers (and users would not know this while editing thanks to missing 'text' tab). Thoughts @bramley ?

@samtuke as @bramley said the Content Areas plugin is a little bit complex and I don't think that will be included by default anytime soon, and I agree that including preheader option would be a good addition as a core functionality.

  1. Cause problems for plaintext users, as the preheader text would unexpectedly be visible for plaintext campaign readers (and users would not know this while editing thanks to missing 'text' tab).

Maybe this can be implemented so that the preheader is only entered in the HTML version of the email. Additionally, enabling the Text tab by default would also be an option.

I think at this point this PR is successful in creating an idea on what the preview in most mail clients would be. I would also like to hear what @bramley thinks before including this in this release.

@samtuke
Copy link
Contributor Author

samtuke commented May 21, 2020

Thanks @suelaP.

Maybe this can be implemented so that the preheader is only entered in the HTML version of the email. Additionally, enabling the Text tab by default would also be an option.

Right now I don't see how, as the plaintext version is always generated from HTML after the campaign is queued. Maybe we could do something like: 1. insert preheader from the html editing page 2. Check if text tab is visible 3a. Force generation and saving of text version if text tab is invisible, as part of setting the preheader in HTML 3b If text tab is visible: display warning and link that they should use it and review text version.

Thoughts?

@suelaP
Copy link
Member

suelaP commented May 21, 2020

Right now I don't see how, as the plaintext version is always generated from HTML after the campaign is queued. Maybe we could do something like: 1. insert preheader from the html editing page 2. Check if text tab is visible 3a. Force generation and saving of text version if text tab is invisible, as part of setting the preheader in HTML 3b If text tab is visible: display warning and link that they should use it and review text version.

Thoughts?

Yes, I think that would be the best approach.

@bramley
Copy link
Contributor

bramley commented May 22, 2020

@samtuke @suelaP You might be misunderstanding where the content of a message is stored.

When sending an html-format email or a plain text-format email the content is always in the 'message' field. The 'textcontent' field is used only for a user-entered text alternative when sending an html-formatted email, which isn't of interest for generating the preview text.

The issue then is deciding whether the message field actually contains html or text. Elsewhere phplist compares the field with the result of using strip_tags() to decide whether it contains html.

There are some edge-cases that might not fit into this. Someone can select html-format on the Format tab, disable the editor, and enter plain text in the content field on the Compose tab. phplist will then convert the plain text to html when sending. But for this purpose, using the plain text directly is probably fine.

I still think that the result needs to include wrapping the message field in the template when that is specified.
And I'd like to see some real-world results of the generated preview text for a campaign and the text actually shown in email clients.

@suelaP
Copy link
Member

suelaP commented May 22, 2020

There are some edge-cases that might not fit into this. Someone can select html-format on the Format tab, disable the editor, and enter plain text in the content field on the Compose tab. phplist will then convert the plain text to html when sending. But for this purpose, using the plain text directly is probably fine.

I still think that the result needs to include wrapping the message field in the template when that is specified.
And I'd like to see some real-world results of the generated preview text for a campaign and the text actually shown in email clients.

Thank you for your comments @bramley . There are definitely cases when this would not work as expected and "real-life" usage of this would provide some more accurate feedback.

I will merge this PR so that the functionality is included in 3.5.4 and use this Mantis issue: https://mantis.phplist.org/view.php?id=20212 to keep track of changes that can improve this.

I think breaking it into smaller issues might make it easier for people to invest some time in things that concern them most. We could also add a flag so people can disable it in the future if they don't make much use of it.

@suelaP suelaP merged commit 5921bd5 into master May 22, 2020
2 checks passed
@bramley
Copy link
Contributor

bramley commented May 22, 2020

@suelaP Hi, I suggest this change is not included yet. It is not simply edge-cases but the main use-case of a campaign using a template is not handled, along with replacing placeholders.

I really think that it needs a bit more thought on what it is trying to achieve. I think that the aim is to generate the same, or at least very similar, text as say Yahoo mail or Gmail generate. That is what I meant by wanting to see some real-world examples. If the generated text is not similar enough then this change isn't going to be useful.

@michield
Copy link
Member

Maybe we should use a constant to enable it or not, so it can be selectively reviewed by more people?

@michield michield deleted the add-message-preview branch October 23, 2022 17:39
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

4 participants