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

Load other font weights and styles for the body-font #2396

Conversation

2ndkauboy
Copy link
Contributor

What?

Using a font name like Google_Karla_400 for body-font, it would only load the 400 font weight. When using Google_Karla_700 for another property, it would then load both the 400 and 700 fon weights. But since in CSS you could make something bold or italic, the font weight and/or style for this text might be missing, as it was not loaded automatically by using it in another property.

This PR will make sure that the most common font weights and font styles will be loaded for the body font.

Screenshots

The Karla font with font-weight: 400 rendered in bold - looks squashed:
Karla-Font

The Karla font with font-weight: 700 rendered in bold - looks perfect:
Karla-Font-700

Using a font name like `Google_Karla_400` for `body-font`, it would
only load the 400 font weight. When using `Google_Karla_700` for
another property, it would then load both the 400 and 700 fon weights.
But since in CSS you could make something bold or italic, the font
weight and/or style for this text might be missing, as it was not
loaded automatically by using it in another property.

This PR will make sure that the most common font weights and font
styles will be loaded for the body font.
@2ndkauboy
Copy link
Contributor Author

I wasn't able to (easily) find out, if the Montserrat font was also used in italic somewhere in CSS. But it might be a good idea to also load it in different styles, like this:

"headings-font": "Google_Montserrat_400,400i,500,500i,700,700i"

@BC-krasnoshapka BC-krasnoshapka requested a review from a team October 16, 2023 09:48
@BC-krasnoshapka
Copy link
Contributor

hi @2ndkauboy , looks reasonable. can you please update CHANGELOG file also? thanks

Copy link
Contributor

@bc-alexsaiannyi bc-alexsaiannyi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@BC-krasnoshapka BC-krasnoshapka left a comment

Choose a reason for hiding this comment

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

CHANGELOG should be updated in this PR

@BC-krasnoshapka BC-krasnoshapka merged commit 8090f58 into bigcommerce:master Jul 4, 2024
3 checks passed
@mattcoy-arcticleaf
Copy link
Contributor

This is not a valid value (since it's not an option in the schema) and this will slow down the PageSpeed, potentially for no reason.

I would recommend reconsidering this change.

@BC-krasnoshapka

@bc-alexsaiannyi
Copy link
Contributor

Hi @mattcoy-arcticleaf. Thank you for bringing that up. Obviously, you're right! What do you think if we can follow up the original contributor and extend Google_Karla_400 with 700 version and delete 2 other italic versions (with respectful changes in schema)? It will probably still slow down the PageSpeed a bit, but not as much as having 4 fonts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants