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

Custom Fonts #39

Merged
merged 16 commits into from
Jun 5, 2019
Merged

Custom Fonts #39

merged 16 commits into from
Jun 5, 2019

Conversation

jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Jun 3, 2019

All Submissions:

Changes proposed in this Pull Request:

This PR introduces support for custom fonts from Google Fonts, Hoefler & Co., Typekit, and Fonts.com. There is a new Typography section in the Customizer where all new fields are found. There are two font families which can be set: Header and Body, which correspond to the theme's Sass variables $font__heading and $font__body. There is also a "fallback stack" select for each which specifies the websafe stack to fall back to. For the moment there are only two fallback options (serif & san-serif), but we could expand to include everything here: https://www.cssfontstack.com/. If the Header Font is set to "My Custom Font" and Header font fallback stack is "San Serif," the full output should look like this:

font-family: My Custom Font, Arial,Helvetica Neue,Helvetica,sans-serif

There are two text fields for import code, which is provided by the services. Here are examples from all four services:

<link type="text/css" rel="stylesheet" href="//fast.fonts.net/cssapi/06088785-3e45-466c-87e4-110b94b1a190.css"/>
<link href="https://fonts.googleapis.com/css?family=Varela+Round&display=swap" rel="stylesheet">
<link rel="stylesheet" href="https://use.typekit.net/jiw3zxe.css">
<link type="text/css" rel="stylesheet" href="//fast.fonts.net/cssapi/06088785-3e45-466c-87e4-110b94b1a190.css"/>

I opted for two to accommodate situations where header and body come from different services, but this is arguably bad practice, and we might consider limiting to just one service.

To sanitize the input, the theme extracts the URL from the embed code, verifies that it is from one of the four supported services, and then reconstructs the import tag (following AMP convention listed here). Because of this approach, pasting in just the URL (e.g. https://fonts.googleapis.com/css?family=Noto+Sans+HK|Roboto+Condensed&display=swap) will work just as well as the full link code. Similarly, @import style code should also work since the URL is in the code, e.g.

<style>
@import url('https://fonts.googleapis.com/css?family=Noto+Sans+HK|Roboto+Condensed&display=swap');
</style>

This PR should be completely AMP compatible.

How to test the changes in this Pull Request:

Google Fonts is the easiest to test with since no account is required. Go to https://fonts.google.com/, choose two fonts and click the + icons next to each. Grab the embed code, and past into the first Font Import Code field. Set Header Font and Body Font to the full font names as listed in Google Fonts (e.g. Noto Sans HK, Roboto Condensed). You should see the fonts immediately in the customizer preview.

I have access to accounts for the other three services, so I can help set up font sets for testing. Just get in touch with me when you begin testing.

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 jeffersonrabb self-assigned this Jun 3, 2019
@jeffersonrabb jeffersonrabb added the [Status] Needs Review The issue or pull request needs to be reviewed label Jun 3, 2019
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

This is looking really cool, @jeffersonrabb! Nothing major on the feedback front from me; let me know if you'd like me to turn any of them into separate issues.

I've verified it works for me with Google Fonts, Fonts.com, Hoefler and Typekit.

inc/customizer.php Outdated Show resolved Hide resolved
inc/customizer.php Outdated Show resolved Hide resolved
inc/customizer.php Outdated Show resolved Hide resolved
inc/customizer.php Outdated Show resolved Hide resolved
inc/typography.php Outdated Show resolved Hide resolved
inc/customizer.php Outdated Show resolved Hide resolved
inc/customizer.php Outdated Show resolved Hide resolved
inc/typography.php Outdated Show resolved Hide resolved
inc/typography.php Outdated Show resolved Hide resolved
inc/typography.php Outdated Show resolved Hide resolved
Copy link
Contributor

@claudiulodro claudiulodro left a comment

Choose a reason for hiding this comment

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

Works great! I think this should meet our needs nicely. Just a few non-blocking things I noticed/suggestions:

  • The fields are very sparse. Some descriptive text and/or examples would have been helpful. I had your explanation in the PR to get me through it, but our users will just have a series of fields with no handholding. I think we might get people posting the link, surrounding <style> tags, etc. It wasn't super clear to me what specifically was meant by "the import tag".
    Something like this would have clarified a lot:

Paste the import tag provided by the font service. Supported services are Fonts.com, Typekit, Google Fonts, and Typography.com.
e.g. @import url('https://fonts.googleapis.com/css?family=Lora|Pacifico&display=swap');

  • We have "Header font" and "Body font family". Should those be standardized to either both font or font family?
  • An example of what a valid font field value is would also be helpful.:

Enter the name of the font you would like to use for body text, as provided by the font service.
e.g. Pacifico

inc/typography.php Outdated Show resolved Hide resolved
@jeffersonrabb
Copy link
Contributor Author

@claudiulodro does 5d4acf6 address the concerns about customizer field labelling and descriptions?

Copy link
Contributor

@claudiulodro claudiulodro left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed. :)

@jeffersonrabb
Copy link
Contributor Author

Ok, I think I've addressed everything!

We should open issues for #39 (comment) and implementing custom fonts in the Editor.

@laurelfulford
Copy link
Contributor

:shipit: !

I can take care of creating the ticket for the editor, and the one for #39 (comment)

@jeffersonrabb jeffersonrabb merged commit 429dec3 into master Jun 5, 2019
@jeffersonrabb jeffersonrabb deleted the try/custom-fonts branch June 5, 2019 18:06
@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants