-
Notifications
You must be signed in to change notification settings - Fork 602
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
Remove Oswald logo font to reduce number of unique fonts, improve pe… #1375
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should note that this is a breaking change of sorts. It's not serious, but here is the scenario: If someone is using a logo font, their current configuration is set to use one of Google_Oswald_400
or Google_Oswald_300
. When they upgrade Cornerstone, their configuration will be using a value that no longer exists in the theme, so the next time they use Theme Editor / Store Design, it will select one of the new values by default (probably Google_Montserrat_400
because it is listed first), and when they save, it will save that value.
We have to be comfortable with the logo font for stores changing after upgrade and upon the next Theme Editor save in order to proceed with this PR.
We should test this upgrade behavior as well to make sure I've described it accurately.
I have updated this PR to make it a less breaking change, by keeping the original fonts around as selectable options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still doesn't guarantee that we'll end up with only two fonts on the page. There are many font options available for different font settings, and we will always load the union of all the font selections.
Another approach would be to eliminate this setting entirely and change the CSS to use one of the other font settings, such as headings-font
: https://github.com/bigcommerce/cornerstone/blob/master/schema.json#L134
@mattolson ♻️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @Ubersmake Can you test this out and confirm my understanding of the configuration upgrade process?
@bookernath Needs rebase |
💚 |
…rformance Most stores will update a logo image, so having a unique font just for the text logo is a waste of performance in the browser. Changing logo font to one of the existing fonts to reduce download and lookups of fonts.
…rformance
What?
Most stores will upload a logo image, so having a unique font just for the text logo is a waste of bandwidth in the browser.
Changing logo font to one of the existing fonts to reduce download and lookups of fonts.
Tickets / Documentation
Screenshots (if appropriate)