feat: font picker #210

jeffersonrabb · 2020-05-18T21:05:06Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Add font selection to the Newsletters plugin. Adds a Typography sidebar panel with SelectControls for choosing body and header fonts. The options are all web-safe fonts, described in screenshots in #198. Selected fonts will immediately be used in the editor and in rendered emails.

Note: the editor font switching uses CSS variables, which are not supported in IE11: https://caniuse.com/#feat=css-variables.

Closes #198

How to test the changes in this Pull Request:

Create a Newsletter.
Experiment with different changes to the Header and Body fonts in the Typography sidebar panel.
Observe the typography in the editor changes to reflect the selections.
Send yourself a test email, observe the correct fonts are used.
View test emails in a variety of email clients. Verify the correct fonts are seen everywhere.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

jeffersonrabb · 2020-05-19T12:27:58Z

@laurelfulford could you take a look at this PR with two particular questions in mind?

Is our list of fonts correct? It's a very safe list, although reading https://templates.mailchimp.com/design/typography/ seems to suggest that there might be opportunities to stretch out a bit more.
What is the best strategy for applying the selected fonts to the Gutenberg editor? In the current state of the PR the fonts will be applied to the actual email, but there is no change in the editor.

jeffersonrabb · 2020-05-19T12:30:55Z

@adekbadek before I get too far along on this PR I wanted to check if the approach runs afoul of #132? This PR creates a new sidebar panel for typography—should this be override-able in other ESP implementations, or should it be universal?

laurelfulford · 2020-05-20T00:27:12Z

Is our list of fonts correct? It's a very safe list, although reading https://templates.mailchimp.com/design/typography/ seems to suggest that there might be opportunities to stretch out a bit more.

The only potential issue with stretching the font list has more to do with the experience editing the newsletter than those receiving it. If we're relying strictly on system fonts, it means someone will have to have a font installed to see it -- and though this is fine for the emails (we'll have font stacks to make sure there are a few similar options to fall back on if they don't have the first font), if someone creating a newsletter doesn't have the font installed, it won't preview the right one for them.

We could get around this with some education/expectation settings, though, maybe even wrangle some links to download the fonts if we do hear from any publishers. The benefits of stretching the list a bit -- and providing some more variety! -- could be worth the possible bit of friction.

We could also just start with the short "guaranteed" list for now, and build on it after the feature is launched :)

What is the best strategy for applying the selected fonts to the Gutenberg editor? In the current state of the PR the fonts will be applied to the actual email, but there is no change in the editor.

Since we're just looking at applying these styles to the editor (vs the front-end of the site), I think it could be a good opportunity to experiment with CSS variables, to follow what's being used in Global Styles in Gutenberg.

We'd just need a change whatever the variable values are, both on editor load (if they're editing an existing newsletter), and on the fly via JavaScript if they mess with the font picker.

We're currently not using CSS Variables in the theme because they aren't supported by IE11. It seems unlikely that any publishers would be using IE11 to edit their site, but if it happens it would just mean that they would not see a change in the preview when they changed the fonts.

adekbadek · 2020-05-20T09:46:06Z

This PR creates a new sidebar panel for typography—should this be override-able in other ESP implementations, or should it be universal?

In the end the font rendering depends on the email client, so I think this feature is universal.

We should add a note though that it's not guaranteed that the font will be rendered as intended in some email clients.

laurelfulford · 2020-05-21T23:58:24Z

I pushed an update to switch the editor to use CSS variables for the header and body fonts.

With those updates, you can switch the fonts by adding the following JavaScript to the console:

document.documentElement.style.setProperty('--body-font', 'chalkboard');

or

document.documentElement.style.setProperty('--header-font', 'georgia');

The defaults for the variables are both set to an Helvetica/Arial font stack, but when the fonts aren't set to the default, something like the above would need to fire on page load (for editing existing newsletters), and when a new font is selected.

I've also updated the styles so the font changes shouldn't affect the placeholder text (like the "Posts Inserter" header, or blocks like the Image block before you've uploaded anything).

laurelfulford

In testing, the fonts change as expected, and when I edit a newsletter, save it, and return to the editor, the font selections are saved.

A couple things I noticed:

'System' as is isn't a font -- when I pick it, the preview switches the font to a serif font in the editor. I've normally seen that referring to a specific 'system font stack' that includes similar fonts for OSX, Windows, and Linux -- like -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Helvetica, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"; from here. (This is what the theme uses as the default header font). I'm not sure if it'd be better to update the option to load that stack, or remove it (and/or use it as the fallback list for the sans-serif fonts) since it kind of overlaps the other options that are there.

Also, a couple of the blocks -- buttons, image captions -- are picking up the body font in the editor, but not picking up any font in the actual email (there, they're using the original Ubuntu, Helvetica, Arial, sans-serif default). I'm not sure which is actually correct -- it might just be that I have it wrong in the editor -- but wanted to confirm before I make any changes:

Editor:

Email:

Lastly, something I know I have to fix: the links nested in h1-h6 (like the post titles) are picking up the body font in the editor, rather than the header font. They're displaying correctly in the email; I can push up a change for this shortly, but wanted to record the "why".

laurelfulford · 2020-05-30T00:24:42Z

Also, a couple of the blocks -- buttons, image captions -- are picking up the body font in the editor, but not picking up any font in the actual email

Jeff and I chatted about this and decided the email should be considered correct here, so I'll be following up shortly with an update to the editor to fix it there.

jeffersonrabb · 2020-06-04T08:18:56Z

'System' as is isn't a font -- when I pick it, the preview switches the font to a serif font in the editor.

Opted to simply remove System in 6564dad.

jeffersonrabb · 2020-06-04T08:20:00Z

Also, a couple of the blocks -- buttons, image captions -- are picking up the body font in the editor, but not picking up any font in the actual email (there, they're using the original Ubuntu, Helvetica, Arial, sans-serif default). I'm not sure which is actually correct -- it might just be that I have it wrong in the editor -- but wanted to confirm before I make any changes:

Fixed in f212f98

jeffersonrabb · 2020-06-04T08:22:55Z

We should add a note though that it's not guaranteed that the font will be rendered as intended in some email clients.

For now we're offering only email-safe fonts as described by Mailchimp, so I would think there's no need for a warning. @adekbadek have you seen cases where these don't render in email clients?

jeffersonrabb · 2020-06-04T08:25:52Z

Resolved an issue where emails sent immediately after a font change do not reflect the change, in fe0e4b1.

Set the default fonts to Arial/Georgia in 3dc85bd

The PR should be ready for review again cc: @adekbadek @laurelfulford

adekbadek

Works well, I just have two remarks.

adekbadek · 2020-06-04T09:42:41Z

src/newsletter-editor/typography/index.js

+	{
+		value: null,
+		label: __( 'Monospace', 'newspack-newsletters' ),
+		disabled: true,


Why are these three disabled option in the select? Also, two of them are not in the backend list of fonts.

Serif, Sans Serif and Monotype are category groupings, not actual fonts. It's meant to match this screenshot, although without the indentation. Actually all three of these should be absent from the list of fonts—which was the one you did find?

'Serif' is on the PHP list

category groupings

SelectControl does not support optgroup :/

'Serif' is on the PHP list

Whoops: 7bb8e8b

includes/class-newspack-newsletters.php

jeffersonrabb · 2020-06-04T16:20:51Z

@adekbadek pointed out that the mockups call for optgroups in the font SelectControls. The SelectControl component has no support for optgroups at the moment (feature request here), so I've added this in f487d7f

adekbadek

😎

laurelfulford

👍 LGTM!

# [1.1.0](v1.0.1...v1.1.0) (2020-06-09) ### Bug Fixes * prevent deduplication in layout picker preview ([4a5721e](4a5721e)), closes [#199](#199) * **posts-inserter:** prevent empty featured image rendering ([56a97c4](56a97c4)), closes [#201](#201) * resolve excerpt length issue ([d40f98a](d40f98a)) * situation that overwrote mc campaign id ([8ecf564](8ecf564)) * update editor block paddings ([#195](#195)) ([eb100b8](eb100b8)) * when trashing post allow mailchimp api call to fail silently ([7e2d9c4](7e2d9c4)) * when trashing post allow mailchimp api call to fail silently ([8257451](8257451)) ### Features * add layouts management ([#165](#165)) ([446674f](446674f)), closes [#31](#31) [#216](#216) * font picker ([#210](#210)) ([0234bf9](0234bf9))

matticbot · 2020-06-09T13:59:46Z

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jeffersonrabb assigned jeffersonrabb and laurelfulford May 19, 2020

jeffersonrabb force-pushed the add/font-picker branch from 1ceca16 to b90f985 Compare May 26, 2020 12:40

jeffersonrabb requested review from adekbadek and laurelfulford May 26, 2020 12:59

jeffersonrabb marked this pull request as ready for review May 26, 2020 12:59

jeffersonrabb added [Status] Needs Review [Status] Needs Design Review labels May 26, 2020

laurelfulford requested changes May 27, 2020

View reviewed changes

jeffersonrabb requested a review from laurelfulford June 4, 2020 08:23

jeffersonrabb removed the [Status] Needs Design Review label Jun 4, 2020

adekbadek reviewed Jun 4, 2020

View reviewed changes

adekbadek approved these changes Jun 4, 2020

View reviewed changes

github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Jun 4, 2020

jeffersonrabb added [Status] Needs Review and removed [Status] Approved Ready to merge labels Jun 4, 2020

jeffersonrabb requested a review from adekbadek June 4, 2020 16:21

adekbadek approved these changes Jun 4, 2020

View reviewed changes

github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Jun 4, 2020

jeffersonrabb and others added 16 commits June 5, 2020 06:28

feat: typography sidebar, basic implementation

a9330d7

feat: update editor styles to use variables for fonts

56679e3

feat: make sure placeholders don't inherit custom fonts

df0b771

feat: editor fonts reflect typography selections

649e86f

feat: make sure links in editor inherit the correct font

b4e29db

feat: improve caption, button font previews

cdf0c8e

fix: remove system font option

2822574

fix: update typography via custom endpoint

de36775

feat: default font pairing - arial/georgia

cb9edb2

feat: font family for buttons and image captions

9156541

feat: default editor typography for placeholders and other ui

1ec9ad7

docs: explanation of custom typography endpoints

6fe2882

fix: remove incorrect font from list

2b44b9a

feat: select control with optgroups for font picker

ac5f5de

fix: return null if optgroups is empty

741b778

feat: improvements to font stacks

53cd2c2

jeffersonrabb force-pushed the add/font-picker branch from 3b13690 to 53cd2c2 Compare June 5, 2020 10:28

laurelfulford approved these changes Jun 5, 2020

View reviewed changes

jeffersonrabb merged commit 0234bf9 into master Jun 5, 2020

matticbot added the released label Jun 9, 2020

adekbadek deleted the add/font-picker branch August 5, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: font picker #210

feat: font picker #210

jeffersonrabb commented May 18, 2020 •

edited

Loading

jeffersonrabb commented May 19, 2020

jeffersonrabb commented May 19, 2020

laurelfulford commented May 20, 2020

adekbadek commented May 20, 2020 •

edited

Loading

laurelfulford commented May 21, 2020

laurelfulford left a comment

laurelfulford commented May 30, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

adekbadek left a comment

adekbadek Jun 4, 2020

jeffersonrabb Jun 4, 2020

adekbadek Jun 4, 2020

adekbadek Jun 4, 2020

jeffersonrabb Jun 4, 2020

jeffersonrabb commented Jun 4, 2020 •

edited

Loading

adekbadek left a comment

laurelfulford left a comment

matticbot commented Jun 9, 2020

feat: font picker #210

feat: font picker #210

Conversation

jeffersonrabb commented May 18, 2020 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

jeffersonrabb commented May 19, 2020

jeffersonrabb commented May 19, 2020

laurelfulford commented May 20, 2020

adekbadek commented May 20, 2020 • edited Loading

laurelfulford commented May 21, 2020

laurelfulford left a comment

Choose a reason for hiding this comment

laurelfulford commented May 30, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

jeffersonrabb commented Jun 4, 2020

adekbadek left a comment

Choose a reason for hiding this comment

adekbadek Jun 4, 2020

Choose a reason for hiding this comment

jeffersonrabb Jun 4, 2020

Choose a reason for hiding this comment

adekbadek Jun 4, 2020

Choose a reason for hiding this comment

adekbadek Jun 4, 2020

Choose a reason for hiding this comment

jeffersonrabb Jun 4, 2020

Choose a reason for hiding this comment

jeffersonrabb commented Jun 4, 2020 • edited Loading

adekbadek left a comment

Choose a reason for hiding this comment

laurelfulford left a comment

Choose a reason for hiding this comment

matticbot commented Jun 9, 2020

jeffersonrabb commented May 18, 2020 •

edited

Loading

adekbadek commented May 20, 2020 •

edited

Loading

jeffersonrabb commented Jun 4, 2020 •

edited

Loading