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.

Sign up for GitHub

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

Font Library: Set filename using the font face data to the font assets #58474

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Font Library: Set filename using the font face data to the font assets #58474

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28ee436
Set filename using the font face data to the font assets
matiasbenedetto Jan 30, 2024
9737488
reduce the number of properties used to create a filename
matiasbenedetto Jan 31, 2024
0801519
Merge branch 'trunk' into try/set-filename-for-font-collection-faces
mikachan Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/edit-site/src/components/global-styles/font-library-modal/font-collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,13 @@ function FontCollection( { slug } ) {
setNotice( null );

const fontFamily = fontsToInstall[ 0 ];

try {
if ( fontFamily?.fontFace ) {
await Promise.all(
fontFamily.fontFace.map( async ( fontFace ) => {
if ( fontFace.src ) {
fontFace.file = await downloadFontFaceAssets(
fontFace.src
);
fontFace.file =
await downloadFontFaceAssets( fontFace );
}
} )
);
Expand Down
34 changes: 29 additions & 5 deletions packages/edit-site/src/components/global-styles/font-library-modal/utils/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { formatFontFamily } from './preview-styles';
*/
const { File } = window;

const { kebabCase } = unlock( componentsPrivateApis );

export function setUIValuesNeeded( font, extraValues = {} ) {
if ( ! font.name && ( font.fontFamily || font.slug ) ) {
font.name = font.fontFamily || font.slug;
Expand Down Expand Up @@ -144,7 +146,6 @@ export function getDisplaySrcFromFontFace( input, urlPrefix ) {

export function makeFontFamilyFormData( fontFamily ) {
const formData = new FormData();
const { kebabCase } = unlock( componentsPrivateApis );

const { fontFace, category, ...familyWithValidParameters } = fontFamily;
const fontFamilySettings = {
Expand Down Expand Up @@ -226,12 +227,34 @@ export async function batchInstallFontFaces( fontFamilyId, fontFacesData ) {
return results;
}

/*
* Makes a font face filename based on the font face properties.
*/
function makeFileNameFromFontFace( fontFace ) {
const properties = [
'fontFamily',
'fontStyle',
'fontWeight',
'unicodeRange',
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that fontStretch is also needed here to be more in line with the css spec for font matching.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming these properties are concatenated to create the final Google Fonts file name only. Then I would suggest we leave out fontStretch. I don't think we need every property in the file name and it'd get looooonnnnnggg.

Copy link
Member

@mikachan mikachan Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could slightly reduce the length of these filenames by using aliases for the font styles, something like:

  • italic = ital or it
  • normal = norm or nor
  • oblique = obli or obl

It's only a tiny change but it may make a difference on particularly long file names.

We could also consider using pascal-case on the font family name (e.g. AROneSans rather than ar-one-sans, similar to how they're saved in the Google Fonts directory) to further reduce the filenames. The properties could still be separated by dashes, like AdLaMDisplay-normal-400.woff2 instead of ad-la-m-display-normal-400.woff2.

Edit: There's some prior art here if it's helpful, which uses [font-family]-[version]-[variant]-[font-weight][font-style], e.g. advent-pro-v28-latin-100italic.woff2. This is even longer, so maybe we don't need to worry about the filename length too much!

];
let name = properties.reduce( ( acc, property ) => {
if ( fontFace?.[ property ] ) {
acc += `${ fontFace[ property ] }-`;
}
return acc;
}, '' );
// Remove the last dash
name = name.slice( 0, -1 );
// Slugify the name
return kebabCase( name );
}

/*
* Downloads a font face asset from a URL to the client and returns a File object.
*/
export async function downloadFontFaceAssets( src ) {
export async function downloadFontFaceAssets( fontFace ) {
// Normalize to an array, since `src` could be a string or array.
src = Array.isArray( src ) ? src : [ src ];
const src = Array.isArray( fontFace.src ) ? fontFace.src : [ fontFace.src ];

const files = await Promise.all(
src.map( async ( url ) => {
Expand All @@ -245,15 +268,16 @@ export async function downloadFontFaceAssets( src ) {
return response.blob();
} )
.then( ( blob ) => {
const filename = url.split( '/' ).pop();
const name = makeFileNameFromFontFace( fontFace );
const extension = url.split( '.' ).pop();
const filename = `${ name }.${ extension }`;
const file = new File( [ blob ], filename, {
type: blob.type,
} );
return file;
} );
} )
);

// If we only have one file return it (not the array). Otherwise return all of them in the array.
return files.length === 1 ? files[ 0 ] : files;
}
Expand Down
Loading