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

EditingToolkit > GlobalStyles: Add new Google fonts #44750

Merged
merged 2 commits into from Aug 20, 2020

Conversation

allilevine
Copy link
Member

@allilevine allilevine commented Aug 6, 2020

Changes proposed in this Pull Request

We've decided to offer additional Google fonts on WordPress.com. This PR adds those fonts to the Editing Toolkit > Global Styles > Font Selection options:

Screen Shot 2020-08-10 at 3 20 51 PM

Testing instructions

To test this change on your sandbox you can apply the patch D47730-code and sandbox the public API, then test on your sandboxed site.

  • Edit a page and click the "A" button in the upper right.
  • The following fonts are available under "Font Selection" in both the Heading Font and Base Font select elements:
    Theme Default
    System Font
    Arvo
    Cabin
    Chivo
    Courier Prime
    Domine
    EB Garamond
    Fira Sans
    Josefin
    Libre Baskerville
    Libre Bodoni
    Libre Franklin
    Lora
    Merriweather
    Montserrat
    Nunito
    Open Sans
    Overpass
    Playfair Display
    Poppins
    Raleway
    Roboto
    Roboto Slab
    Rubik
    Source Sans Pro
    Source Serif Pro
    Space Mono
    Work Sans

Fixes https://github.com/Automattic/dotcom-manage/issues/111

@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D47730-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@allilevine allilevine marked this pull request as ready for review August 10, 2020 15:49
@allilevine allilevine requested review from oandregal and a team as code owners August 10, 2020 15:49
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 10, 2020
@allilevine allilevine requested a review from a team August 10, 2020 18:15
@glendaviesnz
Copy link
Contributor

This tested well for me ... happy to give it a green tick tomorrow once you decide what to do with that font ... I would be inclined to remove it for now and merge others, unless you can track down an alternative one quickly.

@allilevine
Copy link
Member Author

This tested well for me ... happy to give it a green tick tomorrow once you decide what to do with that font ... I would be inclined to remove it for now and merge others, unless you can track down an alternative one quickly.

Thanks! I removed Libre Bodoni. I also corrected the name of "Josefin" to "Josefin Sans".

@glendaviesnz glendaviesnz added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 12, 2020
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

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

I rebased the branch to fix the build issues with the associated diff, and was then able to to sandbox and the new fonts appeared and could be applied.

@glendaviesnz glendaviesnz added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 12, 2020
@glendaviesnz
Copy link
Contributor

given that this doesn't get merged into the calypso app you can probably ignore the e2e test failures

@simison
Copy link
Member

simison commented Aug 12, 2020

Completely unrelated to this PR but shouldn't these be wrapped in translation functions?

const AVAILABLE_FONTS = array(
array(
'label' => 'Theme Default',
'value' => 'unset',
),
array(
'label' => 'System Font',
'value' => self::SYSTEM_FONT,


@allilevine
Copy link
Member Author

Completely unrelated to this PR but shouldn't these be wrapped in translation functions?

@simison Thanks for pointing that out! They should be. Can we open a new issue to add translation functions? I don't want to hold up the font updates.

@pablinos
Copy link
Contributor

This is working for me too. I think we should get some help with deployment. I'm not sure we've got access to the .org repo for example.

Can we open a new issue to add translation functions?

Yes, let's do that as a separate PR. Let's open an issue so we don't forget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants