Skip to content

Commit

Permalink
Fix weight values above 900 not working with Google Fonts (#54339)
Browse files Browse the repository at this point in the history
## What?
This PR fixes issue #54304. As discussed in the issue it was suspected that the values of weights were not being sorted correctly. The weights were being sorted in lexicographical order. 

In order for Google fonts to work, Google Fonts need the values of weights present in the url in sorted order.

Due to this the url which was being generated for Google Fonts which had a value of "1000" in weight was giving an error and the font was not working.

## Why I Think This Solution Works?

Let's assume that we have an object named axes with the property named named "wght" which will have the value of weights as following:

```
const axes = {
  wght: ["100", "200", "300", "400", "500", "600", "700", "800", "900", '1000']
}
```

Let's create another variable named "display" with the value set as "swap" and create another variable named "fontFamily" with the value as "DM Sans" (this is the font which was being used in the issue).

```
const fontFamily = "DM Sans";
const display = "swap";
```

If we call the function using them
```
const FONT_URL = getGoogleFontsUrl(fontFamily, axes, display);
console.log("The font url is: ", FONT_URL)

Output Will Be:
The font url is: https://fonts.googleapis.com/css2?family=DM+Sans:wght@100;1000;200;300;400;500;600;700;800;900&display=swap
```
It will give the incorrect value which was shown in the issue. 

If you see at line number 56 there was no comparison function passed in the sort function which is what was suspected earlier and it was sorting the values lexicographically.

Create another variable inside the function above line 54 where we are updating the value of "url" variable.

```
const variantValues = variants.map((variant) => variant.map(([, val]) => val).join(','))
console.log("Variant Values: ", variantValues)

Output Will Be:
Variant Values: ["100", "200", "300", "400", "500", "600", "700", "800", "900", "1000"]
```

Now call the function again with the same values you will see the values in the correct order but as soon as you call "sort" function it will give you incorrect sorted values which is what is happening in the "url" variable.

```
const variantValues = variants.map((variant) => variant.map(([, val]) => val).join(',')).sort()
console.log("Variant Values: ", variantValues)

Output Will Be:
["100", "1000", "200", "300", "400", "500", "600", "700", "800", "900"] 

(As you can see the values are sorted incorrectly)
```
There was one case when I implemented this solution. When "ital" is also passed then the values are in "ital,wght" format and because of it the sorting was giving incorrect result for this case.

Let's update the "axes" variable which we made previously to contain these values:

```
const axes = {
  wght: ["500","300", "400"],
  ital: ["0","1"]
}
```
Now when we run the function it gives us the result as:
`
https://fonts.googleapis.com/css2?family=DM+Sans:ital,wght@0,500;0,300;0,400;1,500;1,300;1,400&display=swap`

Where as the expected result was: 

`https://fonts.googleapis.com/css2?family=DM+Sans:ital,wght@0,300;0,400;0,500;1,300;1,400;1,500&display=swap`

So, I rewrote the "sort" function to also handle this case.

## What Did I Do To Fix It?

I passed the comparison function inside the "sort" function which was being to ensure that the elements are compared as integers, and thus the sorting will be done numerically.

Which gave me the correct output as url:

`https://fonts.googleapis.com/css2?family=DM+Sans:wght@100;200;300;400;500;600;700;800;900;1000&display=swap`

With values being sorted correctly and if you visit the link you will see the response instead of the error.

Any thought's or suggestions would be greatly appreciated.
  • Loading branch information
indraantoor committed Aug 28, 2023
1 parent 2ab39ec commit dda62b6
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 1 deletion.
4 changes: 3 additions & 1 deletion packages/font/src/google/get-google-fonts-url.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { sortFontsVariantValues } from './sort-fonts-variant-values'

/**
* Generate the Google Fonts URL given the requested weight(s), style(s) and additional variable axes
*/
Expand Down Expand Up @@ -53,7 +55,7 @@ export function getGoogleFontsUrl(
if (variants.length > 0) {
url = `${url}:${variants[0].map(([key]) => key).join(',')}@${variants
.map((variant) => variant.map(([, val]) => val).join(','))
.sort()
.sort(sortFontsVariantValues)
.join(';')}`
}

Expand Down
35 changes: 35 additions & 0 deletions packages/font/src/google/sort-fonts-variant-values.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { sortFontsVariantValues } from './sort-fonts-variant-values'

describe('sortFontsVariantValues', () => {
it('should correctly compare and return result for plain integer values', () => {
// Testing plain integer values
expect(sortFontsVariantValues('100', '200')).toBe(-100)
expect(sortFontsVariantValues('200', '100')).toBe(100)
expect(sortFontsVariantValues('50', '150')).toBe(-100)
expect(sortFontsVariantValues('150', '50')).toBe(100)
})

it('should correctly compare and return result for comma-separated values', () => {
// Testing "ital,wght" format
expect(sortFontsVariantValues('1,100', '0,200')).toBe(1)
expect(sortFontsVariantValues('0,200', '1,100')).toBe(-1)
expect(sortFontsVariantValues('1,100', '1,200')).toBe(-100)
expect(sortFontsVariantValues('1,200', '1,100')).toBe(100)
expect(sortFontsVariantValues('0,100', '0,200')).toBe(-100)
expect(sortFontsVariantValues('0,200', '0,100')).toBe(100)
})

it('should sort an array of plain integer values correctly', () => {
const unsortedArray = ['100', '1000', '300', '200', '500']
const sortedArray = unsortedArray.slice().sort(sortFontsVariantValues)

expect(sortedArray).toEqual(['100', '200', '300', '500', '1000'])
})

it('should sort an array of values with comma-separated values correctly', () => {
const unsortedArray = ['1,100', '1,200', '0,100', '0,200']
const sortedArray = unsortedArray.slice().sort(sortFontsVariantValues)

expect(sortedArray).toEqual(['0,100', '0,200', '1,100', '1,200'])
})
})
25 changes: 25 additions & 0 deletions packages/font/src/google/sort-fonts-variant-values.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Callback function for sorting font variant values.
* Used as a parameter in `Array.prototype.sort` function to ensure correct sorting.
*/

export function sortFontsVariantValues(valA: string, valB: string) {
// If both values contain commas, it indicates they are in "ital,wght" format
if (valA.includes(',') && valB.includes(',')) {
// Split the values into prefix and suffix
const [aPrefix, aSuffix] = valA.split(',')
const [bPrefix, bSuffix] = valB.split(',')

// Compare the prefixes (ital values)
if (aPrefix === bPrefix) {
// If prefixes are equal, then compare the suffixes (wght values)
return parseInt(aSuffix) - parseInt(bSuffix)
} else {
// If prefixes are different, then compare the prefixes directly
return parseInt(aPrefix) - parseInt(bPrefix)
}
}

// If values are not in "ital,wght" format, then directly compare them as integers
return parseInt(valA) - parseInt(valB)
}

0 comments on commit dda62b6

Please sign in to comment.