CRM: Updating core Noto Sans fonts and adding new CJK fonts #36962

coder-karen · 2024-04-17T19:24:13Z

Fixes https://github.com/Automattic/zero-bs-crm/issues/1782

Proposed changes:

This PR updates the $font_json variable with the five new CJK fonts:
- NotoSansJP (now NotoSansJapanese) -> Japanese
- NotoSansKR (now NotoSansKorean) -> Korean
- NotoSansSC (now NotoSansSimplifiedChinese) -> Simplified Chinese
- NotoSansTC (now NotoSansTaiwan)-> Traditional Chinese - Taiwan
- NotoSansHK (now NotoSansHongKong) -> Traditional Chinese - Hong Kong
It also updates the pdffonts.zip file with new core Noto Sans font versions, which apart from updating several-year-old files also means that the two new currencies added in this PR will be viewable on PDF as well: CRM: Add several new currencies to the CRM currency dropdown #36897

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

https://github.com/Automattic/zero-bs-crm/issues/1782

p1713366111446779-slack-CTXBP902X

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

To test, check out this PR or create a JN site using this PR via the beta tester plugin.
Via the locale settings page - wp-admin/admin.php?page=zerobscrm-plugin-settings&tab=locale - set your currency to be the Azerbaijan Manat ('₼', 'AZN'), or the Kazakhstani Tenge ('₸', 'KZT').
Then, at the bottom of the page re-install the default fonts using the 'refresh' icon.
On an invoice, select to download the PDF. The new currency should now be visible.
In trunk, you won't see the currency on the PDF, only a blank square.
To test CJK, you can test if you are able to add Korean, Chinese, or Japanese characters to (for example) an invoice description, after installing the relevant font.
To see the font via PDF, modify the template with the new font added to the CSS styles, as per this link.

with new CJK fonts

github-actions · 2024-04-17T19:25:15Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

… files

… notifications if an install fails

tbradsha · 2024-04-19T16:09:15Z

projects/plugins/crm/admin/settings/locale.page.php

tbradsha · 2024-04-19T16:15:45Z

projects/plugins/crm/includes/ZeroBSCRM.GeneralFuncs.php

+	$large_font_files = array( 'NotoSansSimplifiedChinese.zip', 'NotoSerifDisplay.zip', 'NotoSans.zip', 'NotoSansDisplay.zip', 'NotoSansTaiwan.zip', 'NotoSansHongKong.zip', 'NotoSansJapanese.zip', 'NooSansKorean.zip', 'NotoSansMono.zip' );
+	// Several large font files may timeout when downloading. Increase the timeout for these files.
+	foreach ( $large_font_files as $file ) {
+		if ( str_ends_with( $url, $file ) ) {
+
+			add_filter( 'http_request_timeout', 'custom_large_font_timeout' );
+		}
+	}
+


This works. Did you consider moving this into retrieve_and_install_specific_font(), though? I think it'd work better here:

jetpack/projects/plugins/crm/includes/jpcrm-fonts.php

Line 479 in 2a2fb25

if ( !zeroBSCRM_retrieveFile( $zbs->urls['extdlfonts'] . $remote_zip_name, $temp_path ) ) {

This would mean you no longer need str_ends_with() (or even a loop), as you have the actual filename already.

Did not consider that, I made an assumption the timeout filter needed to be in the same function but I can see it doesn't! Thank you.

tbradsha · 2024-04-19T16:16:09Z

projects/plugins/crm/includes/ZeroBSCRM.GeneralFuncs.php

+/**
+ * Increase the timeout for large font files.
+ * The timeout is increased to 60 seconds (the default is 5), for large font files.
+ * @return int The timeout in seconds.
+ */
+function custom_large_font_timeout() {
+	return 60;
+}
+


As per the above comment, I'd move this into the jpcrm-fonts.php file.

tbradsha

Overall this works, and it's a huge deal! Great job!

I've left a few small comments. One other thing to consider is how the new CJK fonts show in the dropdown. Right now Japanese makes sense, but "Hong Kong", and "Taiwan" aren't scripts. I'd recommend instead doing something like:

Chinese (Traditional, Hong Kong)
Chinese (Traditional, Taiwan)
Chinese (Simplified)

It looks like there's a cleaned_alphabetical section of list_all_available() that supports display manipulation already.

Co-authored-by: tbradsha <[email protected]>

…c_font, with easier font name checks.

tbradsha · 2024-04-19T19:29:37Z

projects/plugins/crm/admin/settings/locale.page.php

@@ -380,7 +380,7 @@
 											sprintf(
 												/* translators: %s is the URL of the CRM support doc explaining how to add styles to pdf templates. */
 												__( 'Once a new font is installed, <a href="%s" target="_blank">learn more about using the custom fonts in CRM PDFs here</a>.', 'zero-bs-crm' ),
-												esc_url( 'https://kb.jetpackcrm.com/knowledge-base/using-custom-fonts-in-crm-pdfs-e-g-invoice-templates' )
+												esc_url( $zbs->urls['kb-pdf-custom-fonts'] )


coder-karen · 2024-04-19T19:29:43Z

Overall this works, and it's a huge deal! Great job!

Thanks!

It looks like there's a cleaned_alphabetical section of list_all_available() that supports display manipulation already.

Yes, you're right. I unnecessarily modified the file names (zip names, etc) to make the display more readable, but I think that is ok to leave them now that they are changed.
I've modified the UI now.

Only one thing I've spotted isn't working ideally - the new support doc link isn't opening in a new tab for me. I am not seeing what is causing this just now, so may need to come back to this.

tbradsha · 2024-04-19T19:39:58Z

Note that I was getting PHP notices when installing some of the CJK fonts due to certain variants not being present:

Deprecated: file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated in /includes/jpcrm-fonts.php on line 587
Deprecated: file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated in /includes/jpcrm-fonts.php on line 588

Resolved in 9be0b73 by flipping the null check with the file_exists check.

tbradsha · 2024-04-19T19:49:44Z

I unnecessarily modified the file names (zip names, etc) to make the display more readable, but I think that is ok to leave them now that they are changed.

Indeed:

This is less important in my opinion, though, as at least they can now easily discover the font to add.

the new support doc link isn't opening in a new tab for me

Looks like target wasn't an allowed attribute. I've added it here: 836aa2f

@params

… that in the @params in function comment

…s-plus-update-core

coder-karen added 3 commits April 17, 2024 20:12

Updating

931bf19

with new CJK fonts

Updating pdffonts.zip with new core Noto Sans fonts

93b3d1f

changelog

f6a8a4c

coder-karen self-assigned this Apr 17, 2024

coder-karen added the [Status] In Progress label Apr 17, 2024

github-actions bot added the [Plugin] CRM Issues about the Jetpack CRM plugin label Apr 17, 2024

coder-karen marked this pull request as ready for review April 17, 2024 19:34

tbradsha and others added 4 commits April 17, 2024 14:18

Zip contents of folder instead of zipping folder

dd9c655

Updating font zip and file names with new zip names in CDN

79e799f

Adding an increase to the default http_request_timeout for large font…

bb7e49b

… files

Add messaging to indicate large font download time, and a link to the…

24709c7

… notifications if an install fails

github-actions bot added the Admin Page React-powered dashboard under the Jetpack menu label Apr 18, 2024

Merge branch 'trunk' into add/crm/new-cjk-fonts-plus-update-core

2a2fb25

coder-karen added [Status] Needs Team Review and removed [Status] In Progress labels Apr 19, 2024

coder-karen requested a review from a team April 19, 2024 08:36

tbradsha reviewed Apr 19, 2024

View reviewed changes

projects/plugins/crm/admin/settings/locale.page.php Outdated Show resolved Hide resolved

tbradsha reviewed Apr 19, 2024

View reviewed changes

coder-karen and others added 5 commits April 19, 2024 17:39

Update projects/plugins/crm/admin/settings/locale.page.php

9e59a95

Co-authored-by: tbradsha <[email protected]>

Adding support doc info and link

310e000

Moving filter timeout functionality into retrieve_and_install_specifi…

7265b74

…c_font, with easier font name checks.

Modifying font display name for UI (dropdown)

ea98601

Adding support doc link to setupUrlsSlugsEtc function

d4723dc

tbradsha reviewed Apr 19, 2024

View reviewed changes

Fix PHP notice + coding standards

9be0b73

Allow target attribute

836aa2f

tbradsha previously approved these changes Apr 19, 2024

View reviewed changes

coder-karen added 2 commits April 22, 2024 08:25

Making sure params that also expect null as well as a string, include…

1f33308

… that in the @params in function comment

Merge remote-tracking branch 'origin/trunk' into add/crm/new-cjk-font…

eb6128b

…s-plus-update-core

coder-karen dismissed tbradsha’s stale review via eb6128b April 22, 2024 07:26

tbradsha approved these changes Apr 22, 2024

View reviewed changes

coder-karen merged commit 48c697d into trunk Apr 22, 2024
53 checks passed

coder-karen deleted the add/crm/new-cjk-fonts-plus-update-core branch April 22, 2024 12:59

github-actions bot removed the [Status] Needs Team Review label Apr 22, 2024

github-actions bot added this to the crm/6.4.3 milestone Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM: Updating core Noto Sans fonts and adding new CJK fonts #36962

CRM: Updating core Noto Sans fonts and adding new CJK fonts #36962

coder-karen commented Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024

tbradsha commented Apr 19, 2024

tbradsha Apr 19, 2024

coder-karen Apr 19, 2024

tbradsha Apr 19, 2024 •

edited

Loading

tbradsha left a comment

tbradsha Apr 19, 2024

coder-karen commented Apr 19, 2024

tbradsha commented Apr 19, 2024

tbradsha commented Apr 19, 2024

CRM: Updating core Noto Sans fonts and adding new CJK fonts #36962

CRM: Updating core Noto Sans fonts and adding new CJK fonts #36962

Conversation

coder-karen commented Apr 17, 2024 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Apr 17, 2024

tbradsha commented Apr 19, 2024

tbradsha Apr 19, 2024

Choose a reason for hiding this comment

coder-karen Apr 19, 2024

Choose a reason for hiding this comment

tbradsha Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

tbradsha left a comment

Choose a reason for hiding this comment

tbradsha Apr 19, 2024

Choose a reason for hiding this comment

coder-karen commented Apr 19, 2024

tbradsha commented Apr 19, 2024

tbradsha commented Apr 19, 2024

coder-karen commented Apr 17, 2024 •

edited

Loading

tbradsha Apr 19, 2024 •

edited

Loading