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/migrate blockbase font self hosted #6123

Closed
wants to merge 77 commits into from

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Jun 23, 2022

This branch continues on from the custom Jetpack Font Management implementation previously created in Blockbase and moves that to instead leverage fonts that are provided locally by the theme.

The goal is that this works both WITH and WITHOUT the Gutenberg plugin activated.

When Gutenberg is activated it provides a custom Font Provider that handles compiling and enqueuing the CSS.
When Gutenberg is NOT activated that action is done manually.

All of the fonts definitions are kept in Blockbase's theme.json file. This is so font selection in the FSE can still work without Gutenberg.

The process attempts to migrate any font settings made via the customizer, or from an activated child theme that hasn't transitioned, to storing the font opinion as block styles. Additionally the Blockbase font definitions are copied to the user space for child themes that haven't transitioned.

Only (and all) those fonts that are USED (assigned via theme.json files for via Global Styles) should have the font-face styles enqueued. All font faces should be enqueued in the FSE.

Testing:

  • This should be evaluated both WITH and WITHOUT the Gutenberg Plugin
  • This will only work with WordPress 6+
  • Test Blockbase instances that have font customizations made prior to this change (Gutenberg only, since customizations were only available in the customizer with Gutenberg activated)
  • Test child themes with no customizations.
  • Test child themes with customizations made prior to this change (Gutenberg Only)

Before:
image

After:
image

creativecoder and others added 30 commits May 25, 2022 00:09
This reverts commit 24dd5c1.
@jeyip
Copy link
Contributor

jeyip commented Jul 8, 2022

@jeyip thank you for your testing so far! If you are able to give this another whirl next week I would really appreciate it!

Hey my mistake. This totally fell off my radar. Taking a look later today

@jeyip
Copy link
Contributor

jeyip commented Jul 9, 2022

Testing

Requirements

Gutenberg

  • All of the fonts provided by Blockbase are available in the Font family dropdown.
  • Google font families are available in the site editor global styles typography dropdowns
  • Only the selected fonts are downloaded from the google fonts api
  • Only the selected fonts have font face declarations added to inline styles
  • Post title global styles typography customizations behave as expected in the editor + frontend

I think I'm seeing another bug though.

On a completely fresh WordPress instance:

  • with Gutenberg installed
  • with the Blockbase theme activated
  • with no customizer typography previously selected from the customizer
  • with a global typography selected
  • when I select a global styles "Heading" block font family, it's only respected in the editor -- not the frontend.

Screen Shot 2022-07-09 at 1 25 06 AM

Screen Shot 2022-07-09 at 1 25 15 AM

The global styles "Post Title" block font family, however, does work as expected.

@pbking
Copy link
Contributor Author

pbking commented Jul 12, 2022

Hrm, I wasn't able to reproduce this bug. The expected typeface is showing for me in both editor and view.

image

I tried both is this branch and in the /blockbase-3 branch which puts all of the bits for this release together. I wonder what might be different about your environment to make that happen.

@pbking
Copy link
Contributor Author

pbking commented Jul 12, 2022

I'm pushing a change that includes refactoring all of the child themes in this repository to use the new font definition so that if there are no customizations the migration will not be necessary. And also taken the opportunity to evaluate all of the child themes to ensure that the fonts look the same before and after this change.

@jeyip
Copy link
Contributor

jeyip commented Jul 14, 2022

Hrm, I wasn't able to reproduce this bug. The expected typeface is showing for me in both editor and view.

I'll test this again tomorrow. I wonder if there's something specific to my environment 🤔 .

To set expectations, I've been super busy onboarding a new hire and with new team priorities ( we just switched gears and are being redirected to a new project ). Would the themes team be able to help code review this PR? I won't be able to get back to this as often as I'd like, and I wouldn't want the momentum on these code changes to stall.

@pbking
Copy link
Contributor Author

pbking commented Jul 14, 2022

Thanks for the heads up @jeyip and your willingness to help amongst your busy schedule! We've had some eyes on this but it's so big it's hard to give a ✅ . We've done some testing with this branch merged with the other Blockbase 3 issues all bundled together and we're looking at that pretty hard. Hopefully anything will shake out there if nothing else.

@jeyip
Copy link
Contributor

jeyip commented Jul 16, 2022

I'll test this again tomorrow.

Couldn't get to testing today, but it's the first thing I'll do Monday morning

pbking added a commit that referenced this pull request Jul 20, 2022
Refactor/blockbase color admin (#6043)
Moved templates from old folder location to new (#6073)
Blockbase: Implement the Button elements API (#6041)
Blockbase: Implement Comment Block and removed CSS (#6080)
Fix/migrate blockbase font self hosted (#6123)
Blockbase children: update comments block (#6153)
Blockbase: Changed the trigger to render social icons (#6079)
Blockbase: move button padding styles from ponyfill to theme.json (#5901)

Co-authored-by: Grant Kinney <[email protected]>
Co-authored-by: Jeremy Yip <[email protected]>
Co-authored-by: MaggieCabrera <[email protected]>
Co-authored-by: madhusudhand <[email protected]>
@pbking pbking mentioned this pull request Jul 20, 2022
@pbking
Copy link
Contributor Author

pbking commented Jul 20, 2022

Closing: Work merged in #6167

@pbking pbking closed this Jul 20, 2022
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.

3 participants