[WIP] CYS - Core: add integration with the font library #44004

gigitux · 2024-01-23T16:03:51Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes #41924.

This PR implements the support for the Font Library to allow the user to pick different fonts on the Assembler.

Technical Details

Font Library API

The Font Library is based on two post types after the API refactor:
wp_font_family -> contains the data related to the Font Family (e.g.: slug)
wp_font_face -> includes the data of specific font faces of a particular font family.

There is a strict relationship between these two post types. The wp_font_family is the parent post of the wp_font_face. For this reason, it is necessary to create a wp_font_familypost before installing the various font faces.

CYS integration

The fonts installed are defined with the FONT_FAMILY_TO_INSTALL constant.

This constant is created manually by reading the value from the FONT_PAIRINGS_WHEN_AI_IS_OFFLINE constant (source code): we want to install only the necessary fonts to reduce the setup time and not install too many fonts. We could create a logic to generate this variable, but at this time, it isn't worth it, in my opinion.

During the initial setup, the client gets the already installed Font Families (and related Font Face). In this way, we can install only the fonts that aren't already installed. (source code)

After the setup is finished, when the Assembler is loaded, all the fonts are preloaded. (source code)

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Create a new WooCommerce installation with this version.
Ensure the WooCommerce Beta Tester plugin is installed and activated (available on this monorepo).
Head over to /wp-admin/tools.php?page=woocommerce-admin-test-helper and enable customize-store feature flag.
Install this build of Gutenberg that contains the new Font Library API:
gutenberg.zip
Visit the wp-admin/admin.php?page=wc-admin&path=/customize-store.
Open the network tools.
Follow the process.
Ensure that there are several POST requests to the /wp/v2/font-families/ endpoint.
Click "Change fonts" on the left sidebar.
Select different fonts and ensure that the selected font is applied to the first title
Visit the wp-admin/admin.php?page=wc-admin&path=/customize-store.
Open the network tools.
Ensure that no POST requests to the /wp/v2/font-families/ endpoint are done.

Warnings ⚠️

Currently, in some cases, the font assigned to the buttons isn't the correct one [CYS] Ensure the font family for buttons match the one assigned for the content, not the title #44010.
Some E2E tests are failing. The reason is that the Font Library API isn't available on the E2E tests env. Before merging this PR, it is necessary to implement a way to detect if the Font Library isn't available to avoid fetching requests to non-existent endpoints. This work will be done in a dedicated PR.

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

CYS - Core: add integration with the font library.

Comment

github-actions · 2024-01-23T16:32:04Z

Test Results Summary

Commit SHA: da98e3f

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 36s
E2E Tests	244	0	2	62	0	308	17m 5s

⚠️ Warning

Please address the following issues prior to merging this pull request:

FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

…mmerce

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

github-actions · 2024-01-24T17:58:05Z

Hi @albarin, @woocommerce/woo-fse

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

nefeline

Thanks for working on this, @gigitux ! A few notes on my end:

Whenever running the Assembler for the first time, the default font pairing is not being pre-selected on the sidebar, so it's impossible to determine the one that is currently active:

Screen.Recording.2024-01-25.at.09.44.00.mov

Some font pairings are influencing the size of other font pairings:

Click on Montserrat + Arvo
Click on Albert Sans + Lora, the size is the same.
Click on Rubik + Inter and you'll see the font is bigger.
Click back on Albert Sans + Lora and check how the font is bigger when compared to the initial Albert Sans + Lora pairing right after clicking on Montserrat + Arvo

Screen.Recording.2024-01-25.at.10.01.02.mov

Ideally, the size of the fonts should not be affected by the font family changes, independent of the provided combinations.

Some fonts don't seem to be changing in certain patterns whenever selected, see example:

Screen.Recording.2024-01-25.at.10.03.47.mov

I've also left a note regarding the moment when font installation should be triggered

plugins/woocommerce-admin/client/customize-store/design-without-ai/services.ts

gigitux · 2024-01-25T13:39:46Z

Thanks for working on this, @gigitux ! A few notes on my end:

Whenever running the Assembler for the first time, the default font pairing is not being pre-selected on the sidebar, so it's impossible to determine the one that is currently active:

Screen.Recording.2024-01-25.at.09.44.00.mov
2. Some font pairings are influencing the size of other font pairings:

Click on Montserrat + Arvo

Click on Albert Sans + Lora, the size is the same.

Click on Rubik + Inter and you'll see the font is bigger.

Click back on Albert Sans + Lora and check how the font is bigger when compared to the initial Albert Sans + Lora pairing right after clicking on Montserrat + Arvo

Screen.Recording.2024-01-25.at.10.01.02.mov
Ideally, the size of the fonts should not be affected by the font family changes, independent of the provided combinations.

Some fonts don't seem to be changing in certain patterns whenever selected, see example:

Screen.Recording.2024-01-25.at.10.03.47.mov
I've also left a note regarding the moment when font installation should be triggered

Regarding the points 2 and 3, I guess that it is something related to this issue #44010, but I will double-check them!

Nice catch regarding the first point!

Thanks for your great review! 🙏

gigitux · 2024-01-29T14:36:16Z

WordPress/gutenberg#58222 implements some changes to the API. fcc774b adapt the code with the new shape of the API

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

gigitux · 2024-02-01T09:54:07Z

@nefeline With 10cb26e I did another optimization to improve the font installation speed.

I think that it could make sense to review/merge this PR because it is already pretty big. As follow-up:

we have to implement the async job to install the fonts via PHP -> I will create an issue
we have to fix that the default font pairing is not being pre-selected on the sidebar -> I will create an issue
Check the rest of the issue with the font paring in this: [CYS] Ensure the font family for buttons match the one assigned for the content, not the title #44010 because there could be some correlation.

WDYT?

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

nefeline

@nefeline With 10cb26e I did another optimization to improve the font installation speed.

Excellent! Thanks for your work here, @gigitux.

I think that it could make sense to review/merge this PR because it is already pretty big. As follow-up:

Agreed 🤝 . Adding a note here for future reference the following issues must be addressed in order to ensure a successful implementation of this feature on core:

#44259
#44258
#44257
#44010

With that said, I'm going ahead and approving the PR so we can unblock the work for the aforementioned bug fixes & improvements.

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

gigitux · 2024-02-01T17:06:09Z

@nefeline With 10cb26e I did another optimization to improve the font installation speed.

Excellent! Thanks for your work here, @gigitux.

I think that it could make sense to review/merge this PR because it is already pretty big. As follow-up:

Agreed 🤝 . Adding a note here for future reference the following issues must be addressed in order to ensure a successful implementation of this feature on core:

#44259 #44258 #44257 #44010

With that said, I'm going ahead and approving the PR so we can unblock the work for the aforementioned bug fixes & improvements.

Thanks for the review and for opening these issues! 🚀

* CYS - Core: add integration with the font library * install font only when necessary * refactor logic * add try catch * renaming font * refactor some code * refactor some logic * Add changefile(s) from automation for the following project(s): woocommerce * remove not used import * avoid mutability * improve performance * update name variable * fix naming * fix endpoints after font collection rest controller improvements * use promise.all into the map * improve performance * fix lint error --------- Co-authored-by: github-actions <github-actions@github.com>

CYS - Core: add integration with the font library

0179e08

gigitux linked an issue Jan 23, 2024 that may be closed by this pull request

[CYS on Core] Ensure the fonts selected in the Assembler are displayed on the frontend #41924

Closed

gigitux added 7 commits January 23, 2024 18:43

install font only when necessary

10c6cd2

refactor logic

2e48174

add try catch

5f1b46d

renaming font

064ae22

refactor some code

ad616b3

refactor some logic

3791b41

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

6cfe61d

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

gigitux self-assigned this Jan 24, 2024

gigitux added team: Kirigami & Origami focus: customize-your-store Issues related to the Customize Your Store onboarding flow. labels Jan 24, 2024

github-actions and others added 2 commits January 24, 2024 16:15

Add changefile(s) from automation for the following project(s): wooco…

dbce49c

…mmerce

remove not used import

67b0c3e

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jan 24, 2024

gigitux added 5 commits January 24, 2024 17:34

avoid mutability

fadaba7

improve performance

729112a

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

fd02c06

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

update name variable

a8c23ba

fix naming

e534157

gigitux requested review from nefeline and albarin January 24, 2024 17:57

gigitux mentioned this pull request Jan 25, 2024

[CYS - Core] Handling the Font Picker when the Font Library isn't available #44074

Closed

nefeline requested changes Jan 25, 2024

View reviewed changes

plugins/woocommerce-admin/client/customize-store/design-without-ai/services.ts Show resolved Hide resolved

fix endpoints after font collection rest controller improvements

fcc774b

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

5babbac

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

use promise.all into the map

b8e48fa

nefeline mentioned this pull request Jan 30, 2024

[CYS on Core] Conditionally change the default fonts available based on user consent. #44187

Closed

gigitux added 3 commits January 30, 2024 22:21

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

1e66f5a

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

cc249b8

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

improve performance

10cb26e

fix lint error

59063e0

gigitux marked this pull request as ready for review February 1, 2024 11:24

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

78e38ab

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

woocommercebot requested a review from a team February 1, 2024 11:30

gigitux force-pushed the 41924-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend branch from 99376da to 78e38ab Compare February 1, 2024 12:59

nefeline approved these changes Feb 1, 2024

View reviewed changes

Merge branch 'trunk' of github.com:woocommerce/woocommerce into 41924…

da98e3f

…-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend

gigitux enabled auto-merge (squash) February 1, 2024 17:05

gigitux merged commit e0972ef into trunk Feb 1, 2024
23 of 25 checks passed

gigitux deleted the 41924-cys-ensure-the-fonts-selected-in-the-assembler-are-displayed-on-the-frontend branch February 1, 2024 17:16

github-actions bot added this to the 8.7.0 milestone Feb 1, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 1, 2024

gigitux mentioned this pull request Feb 1, 2024

CYS - AI flow: fix regression - no fonts visible #44280

Merged

11 tasks

veljkho added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 2, 2024

This was referenced Feb 9, 2024

CYS - Core: Add async font installation #44473

Merged

[CYS - Core] Unit Test: Add test for the util function to install fonts on the JS side #44590

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] CYS - Core: add integration with the font library #44004

[WIP] CYS - Core: add integration with the font library #44004

gigitux commented Jan 23, 2024 •

edited

github-actions bot commented Jan 23, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited

nefeline left a comment

gigitux commented Jan 25, 2024

gigitux commented Jan 29, 2024

gigitux commented Feb 1, 2024

nefeline left a comment

gigitux commented Feb 1, 2024

[WIP] CYS - Core: add integration with the font library #44004

[WIP] CYS - Core: add integration with the font library #44004

Conversation

gigitux commented Jan 23, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

Technical Details

Font Library API

CYS integration

How to test the changes in this Pull Request:

Warnings ⚠️

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Jan 23, 2024 • edited

Test Results Summary

⚠️ Warning

github-actions bot commented Jan 24, 2024 • edited

nefeline left a comment

Choose a reason for hiding this comment

gigitux commented Jan 25, 2024

gigitux commented Jan 29, 2024

gigitux commented Feb 1, 2024

nefeline left a comment

Choose a reason for hiding this comment

gigitux commented Feb 1, 2024

gigitux commented Jan 23, 2024 •

edited

github-actions bot commented Jan 23, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited