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

Bug 1490392: Reduce web font usage #4967

Merged
merged 10 commits into from Sep 13, 2018

Conversation

Projects
None yet
6 participants
@tkadlec
Contributor

tkadlec commented Sep 11, 2018

This PR addresses https://app.zenhub.com/workspace/o/mdn/sprints/issues/255 by removing Zilla Slab Italic in favor of font synthesis and eliminating Open Sans and using Verdana instead.

This drops our web font count from 6 to 2, decreases our font weight by 39% (211.4kb) and improves reduces the amount of rendering work the browser has to do.

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 11, 2018

@schalkneethling @jwhitlock Killing fonts here. Would love a review.

@@ -1,2 +1,3 @@
{# because this is non-blocking, preload as early as possible #}
<link rel="preload" href="{{ static('fonts/locales/ZillaSlab-Regular.woff2') }}" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="{{ static('fonts/locales/ZillaSlab-Bold.woff2') }}" as="font" type="font/woff2" crossorigin />

This comment has been minimized.

@tkadlec

tkadlec Sep 11, 2018

Contributor

I'm sneaking another preload in here now that we have it down to just the two web fonts. Helps a bit with rendering (about 35% reduction over not preloading for the home page which doesn't have much render work to begin with, but only about 5% reduction over no preload on an interactive example page), but mostly ensures the web fonts are available right away.

@@ -47,7 +47,6 @@ $padding-horizontal: 30px;
color: $accent-light;
display: inline-block;
@include set-font-size(17px);
font-weight: bold;

This comment has been minimized.

@tkadlec

tkadlec Sep 11, 2018

Contributor

Verdana's bold is heavier than Open Sans. I pulled the bold here (and a couple other spots) to minimize the difference.

@include set-site-font-family();
@include set-font-size(15px);

This comment has been minimized.

@tkadlec

tkadlec Sep 11, 2018

Contributor

I don't normally like to reduce the font size below 16px, but Verdana is big. Playing with https://meowni.ca/font-style-matcher/ shows that 15px (with some tiny tweaks like the line-height and some letter-spacing) is nearly identical to 16px Open Sans.

This comment has been minimized.

@schalkneethling

schalkneethling Sep 13, 2018

Collaborator

You can use $body-font-size here now.

This comment has been minimized.

@tkadlec

tkadlec Sep 13, 2018

Contributor

👍

$small-desktop-ends: (1199px / 16px) * 1em;
$tablet-ends: (1023px / 16px) * 1em;
$mobile-ends: (767px / 16px) * 1em;
$small-mobile-ends: (479px / 16px) * 1em;

This comment has been minimized.

@tkadlec

tkadlec Sep 11, 2018

Contributor

This was necessary to keep the same general width of our content. I tested the layout using devtools and we seem ok with the minor adjustments to our breakpoints.

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 11, 2018

A few side by sides for comparison. (After font changes on the left, before font changes on the right)

screen shot 2018-09-11 at 9 16 04 am
screen shot 2018-09-11 at 9 15 18 am
screen shot 2018-09-11 at 9 15 01 am

@schalkneethling schalkneethling self-requested a review Sep 11, 2018

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 11, 2018

@tkadlec Thanks for the PR. One thing I notice is that no actual font files are removed in this PR. As we are no longer using them, I would suggest you delete them from the codebase as well. Thanks!

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 11, 2018

Heh. I was just realizing the same thing. :)

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 11, 2018

There was a whole bunch of random variations of ZillaSlab that we weren't using anywhere, so I removed those as well now.

@jwhitlock jwhitlock changed the title from Bug 255: Reduce web font usage to Bug 1490392: Reduce web font usage Sep 11, 2018

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Sep 11, 2018

I opened bug 1490392 and updated the title for this issue. We have a bugzilla bot that will see a merge with the text "Bug 255", and will update bug 255, reported 21 years ago, instead of mdn/sprints#255.

@jwhitlock

Looks good, with one nit. I tried some pages locally, and built the production docker images to make sure there aren't some missing paths.

I suspect Gaia is an old MDN project that we can get rid of soon, but can be done after this PR.

@@ -5,4 +5,3 @@ Fonts for Gaia demos
********************************************************************** */
@import 'base/fonts/moztt';
@import 'base/fonts/opensans';

This comment has been minimized.

@jwhitlock

jwhitlock Sep 11, 2018

Member

What is Gaia? I'll search repository history, and ask the writers.

This comment has been minimized.

@schalkneethling

schalkneethling Sep 11, 2018

Collaborator

From what I know, Gaia was the codename for the front-end codebase of Firefox-OS

This comment has been minimized.

@jwhitlock

jwhitlock Sep 11, 2018

Member

@schalkneethling is right: https://developer.mozilla.org/en-US/docs/Archive/B2G_OS/Platform/Gaia

A lot of this was added in 2013, like bug 844531, to support documentation and samples. There's a few samples in the archive, like https://developer.mozilla.org/en-US/docs/Archive/B2G_OS/Firefox_OS_apps/Building_blocks/Slider/2.0. That one has been converted to use external assets, hosted on GitHub.

Removing these may break some samples, but keeping those samples working is a low priority. Future work, including some research to see what will break.

Update: opened https://bugzilla.mozilla.org/show_bug.cgi?id=1490411, looks safe to remove Gaia stuff.

$small-desktop-ends: (1199px / 15px) * 1em;
$tablet-ends: (1023px / 15px) * 1em;
$mobile-ends: (767px / 15px) * 1em;
$small-mobile-ends: (479px / 15px) * 1em;

This comment has been minimized.

@jwhitlock

jwhitlock Sep 11, 2018

Member

Nit: Is it possible to move 15px into a SCSS variable? This change from 16px to 15px would have been a one-line change...

This comment has been minimized.

@schalkneethling

schalkneethling Sep 12, 2018

Collaborator

Agreed. It is dividing by the base body font size. I would suggest updating the variable here https://github.com/mozilla/kuma/blob/master/kuma/static/styles/includes/_vars.scss#L132 to be 15px and then use the variable for consistency.

This comment has been minimized.

@tkadlec

tkadlec Sep 12, 2018

Contributor

All set!

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Sep 11, 2018

I mostly reviewed from the backend, "does this break the build" perspective. I'm not qualified to judge how it looks, or if the new media breakpoints are appropriate. I think a positive review from @schalkneethling is needed to merge.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 12, 2018

@tkadlec I am going to suggest that we add the following to the body element:

-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;

A large portion of our audience will get the benefit and it really makes the copy a lot more readable:

compare

@schalkneethling

Couple of nits to clean up. Thanks @tkadlec

$small-desktop-ends: (1199px / 15px) * 1em;
$tablet-ends: (1023px / 15px) * 1em;
$mobile-ends: (767px / 15px) * 1em;
$small-mobile-ends: (479px / 15px) * 1em;

This comment has been minimized.

@schalkneethling

schalkneethling Sep 12, 2018

Collaborator

Agreed. It is dividing by the base body font size. I would suggest updating the variable here https://github.com/mozilla/kuma/blob/master/kuma/static/styles/includes/_vars.scss#L132 to be 15px and then use the variable for consistency.

@@ -41,6 +41,7 @@ Provides main mixins for use within the MDN theme.
@mixin set-site-font-family() {
font-family: $site-font-family;
letter-spacing: -.05px;

This comment has been minimized.

@schalkneethling

schalkneethling Sep 12, 2018

Collaborator

Nit: Prefer rem units over px for typography

This comment has been minimized.

@tkadlec

tkadlec Sep 12, 2018

Contributor

Should be set here.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 12, 2018

I notice that the newsletter section overrides Verdana with x-locale-heading-primary, zillaslab, "Palatino", "Palatino Linotype", x-locale-heading-secondary, serif. Not sure if there is a specific reason for this, but it looks strange to have a serif mixed in with a sans-serif typeface.

screen shot 2018-09-12 at 11 43 16

The same is happening on production so, was not introduced here but, thought I would mention it. @jwhitlock you know of a specific reason for this?

Update: This seems intentional and works well when the newsletter signup is at the bottom of the page. Just looks a little odd in this instance.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 12, 2018

This is looking good. Thanks @tkadlec - just a couple of items to address, and then this should be good to go. 🎉

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 13, 2018

@tkadlec What are your thoughts on #4967 (comment) ?

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 13, 2018

@schalkneethling I thought I had done that! I remember looking at it in the browser. :/ Anyway...added and pushed now.

@schalkneethling

r+ This is gonna make a noticeable difference in performance I reckon. Cannot wait for font-smooth to be support everywhere. I makes such a massive difference. 🎉

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Sep 13, 2018

@tkadlec is a squash commit OK for these changes?

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 13, 2018

@jwhitlock Yeah, I reckon so.

@jwhitlock jwhitlock merged commit dd88c15 into mozilla:master Sep 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No manifest changes detected

@jwhitlock jwhitlock referenced this pull request Sep 13, 2018

Closed

Consider removing some fonts/variations #255

4 of 6 tasks complete
@a2sheppy

This comment has been minimized.

Contributor

a2sheppy commented Sep 13, 2018

I love the size reduction of page loads here, but there are definitely some text legibility problems introduced by this. Letter spacing is a little too broad, especially for bold type, and is really pretty bad when looking at bold type in the editor, as seen in this screen shot:

image

Previously, this was much more legible, looking something like this (this is sort of a recreation made by tweaking the CSS to restore Open Sans as the body font):

image

@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 14, 2018

Believe we can tweak this to make it better. To be clear, are you saying that wherever Verdana is bold we have issues?

@stephaniehobson

This comment has been minimized.

Contributor

stephaniehobson commented Sep 17, 2018

I think I'd prefer arial to verdana. I think the legibility is more important than matching the old width.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented Sep 18, 2018

I think I'd prefer arial to verdana. I think the legibility is more important than matching the old width.

While font-smooth does improve it, I have to agree that Arial is more legible font-face in general. Not sure if the decision was made by brand or, based on the closest match. @tkadlec @atopal you know?

@atopal

This comment has been minimized.

atopal commented Sep 18, 2018

I think it was made based on closest match. I'm fine with Arial.

joshJarr added a commit to potatolondon/kuma that referenced this pull request Sep 18, 2018

CTA on homepage (#5)
* Bug 1487656 ‑ Comment out the indicator `:matches(…)` block for now

# External links:
- https://bugzil.la/1487656

* Bug 1489981, history button goes to edit instead on dashboard

* Bug 1490392: Reduce web font usage (mozilla#4967)

Squashes several commits:
* Removing Open Sans from Gaia demos
* Fixing linting issue on mixins
* removing open sans from maintenance page
* Tweaking styles now that Verdana is in place to minimize differences
* Preload zilla bold
* Removing actual font files
* Using a variable in the breakpoints
* Using rems for letter-spacing
* Using body-font-size in document
* Font smoothing

* update sub-modules

* bug 1490727: Add stripe 2.7.0 requirement

The Python library for the Stripe payments API.

* bug 1490727: Update to stripe 2.8.0

* stripe 2.7.0 → 2.8.0: Add support for automatic retries

* bug 1490727: Update to stripe 2.8.1

* stripe 2.8.0 → 2.8.1: Sync features with ruby library

* added CTA on homepage

* added local storage functionality to remove cta

* fix newline files

* add context processor for contribution form

* fix mobile view, link mobile to /contribute

* fix linting

* address internal feedback

* rename cta class

* Fixed popover button interaction.

* Fix contibute banner button display on mobile.

* Fix padding and margins on form button

* Fixed popover styling for contribution form.

* Squash - PR feedback and code organisation

* Updated the height of the popover for actions

* Implemented FAQ styles, merged with current master branch. (#6)

* Implemented FAQ styles, merged with current master branch.

* Fix newline linting issue on SVGs

* added newline in js

* Address PR feedback, fix linting errors.

* Fix some minor design feedback, update classnames

* Include some legal copy and feedback form.
@tkadlec

This comment has been minimized.

Contributor

tkadlec commented Sep 18, 2018

Yup....based on similarity. All for switching to Arial. Is there anyone else we should include in this discussion or are we good to go?

@jwhitlock

This comment has been minimized.

Member

jwhitlock commented Sep 19, 2018

I'm OK with the change, not sure about the timing. @atopal, do you want to aim for before or after Oct 1st to ship, or does it mater?

@atopal

This comment has been minimized.

atopal commented Sep 19, 2018

Not time critical, after October 1st is perfectly okay.

joshJarr added a commit to potatolondon/kuma that referenced this pull request Sep 19, 2018

Implement the rest of the user flow for payments. (#9)
* Bug 1487656 ‑ Comment out the indicator `:matches(…)` block for now

# External links:
- https://bugzil.la/1487656

* Bug 1489981, history button goes to edit instead on dashboard

* Bug 1490392: Reduce web font usage (mozilla#4967)

Squashes several commits:
* Removing Open Sans from Gaia demos
* Fixing linting issue on mixins
* removing open sans from maintenance page
* Tweaking styles now that Verdana is in place to minimize differences
* Preload zilla bold
* Removing actual font files
* Using a variable in the breakpoints
* Using rems for letter-spacing
* Using body-font-size in document
* Font smoothing

* update sub-modules

* bug 1490727: Add stripe 2.7.0 requirement

The Python library for the Stripe payments API.

* bug 1490727: Update to stripe 2.8.0

* stripe 2.7.0 → 2.8.0: Add support for automatic retries

* bug 1490727: Update to stripe 2.8.1

* stripe 2.8.0 → 2.8.1: Sync features with ruby library

* bug 1490727: Fix path to American Express logo

* bug 1490727: Remove duplicate code

These should have been removed when moved to the contributions app.

* bug 1490727: Add locale-prefix to contribute

Make /contribute a locale-prefixed URL, and add tests for both the
enabled and disabled view.

* added CTA on homepage

* added local storage functionality to remove cta

* fix newline files

* add context processor for contribution form

* fix mobile view, link mobile to /contribute

* fix linting

* address internal feedback

* rename cta class

* Fixed popover button interaction.

* Fix contibute banner button display on mobile.

* Fix padding and margins on form button

* Fixed popover styling for contribution form.

* Squash - PR feedback and code organisation

* Updated the height of the popover for actions

* Implemented FAQ styles, merged with current master branch. (#6)

* Implemented FAQ styles, merged with current master branch.

* Fix newline linting issue on SVGs

* added newline in js

* Address PR feedback, fix linting errors.

* Fix some minor design feedback, update classnames

* Include some legal copy and feedback form.

* fix for celery worker in mdn contribution thank you email sending

* CI build failures

* CI whitespace fix

* Addressed design feedback for legal copy

* Updated Copy

* fix for Decimal rounding in ContributionForm

* Call to action design feedback

* Fix grammar on legal copy

* Update for user testing branch (#7)

* bug 1490727: Minimize SVG with svgo

* bug 1490727: Use blocks instead of URLs

Use a Jinja2 {% block %} to remove the popup from the stand-alone
contribution page.

* bug 1490727: Reformat translations

Use {% trans %} for multi-line strings, and format them to limited
column width. Extract the URLs from the translated strings, so
translators won't mess with them. Use <em> instead of <i>.

* bug 1490727: Move standard import above others

joshJarr added a commit to potatolondon/kuma that referenced this pull request Sep 19, 2018

Merge in a few backend fixes for email (#10)
* Bug 1487656 ‑ Comment out the indicator `:matches(…)` block for now

# External links:
- https://bugzil.la/1487656

* Bug 1489981, history button goes to edit instead on dashboard

* Bug 1490392: Reduce web font usage (mozilla#4967)

Squashes several commits:
* Removing Open Sans from Gaia demos
* Fixing linting issue on mixins
* removing open sans from maintenance page
* Tweaking styles now that Verdana is in place to minimize differences
* Preload zilla bold
* Removing actual font files
* Using a variable in the breakpoints
* Using rems for letter-spacing
* Using body-font-size in document
* Font smoothing

* update sub-modules

* bug 1490727: Add stripe 2.7.0 requirement

The Python library for the Stripe payments API.

* bug 1490727: Update to stripe 2.8.0

* stripe 2.7.0 → 2.8.0: Add support for automatic retries

* bug 1490727: Update to stripe 2.8.1

* stripe 2.8.0 → 2.8.1: Sync features with ruby library

* bug 1490727: Fix path to American Express logo

* bug 1490727: Remove duplicate code

These should have been removed when moved to the contributions app.

* bug 1490727: Add locale-prefix to contribute

Make /contribute a locale-prefixed URL, and add tests for both the
enabled and disabled view.

* added CTA on homepage

* added local storage functionality to remove cta

* fix newline files

* add context processor for contribution form

* fix mobile view, link mobile to /contribute

* fix linting

* address internal feedback

* rename cta class

* Fixed popover button interaction.

* Fix contibute banner button display on mobile.

* Fix padding and margins on form button

* Fixed popover styling for contribution form.

* Squash - PR feedback and code organisation

* Updated the height of the popover for actions

* Implemented FAQ styles, merged with current master branch. (#6)

* Implemented FAQ styles, merged with current master branch.

* Fix newline linting issue on SVGs

* added newline in js

* Address PR feedback, fix linting errors.

* Fix some minor design feedback, update classnames

* Include some legal copy and feedback form.

* fix for celery worker in mdn contribution thank you email sending

* CI build failures

* CI whitespace fix

* Addressed design feedback for legal copy

* Updated Copy

* fix for Decimal rounding in ContributionForm

* Call to action design feedback

* Fix grammar on legal copy

* Update for user testing branch (#7)

* bug 1490727: Minimize SVG with svgo

* bug 1490727: Use blocks instead of URLs

Use a Jinja2 {% block %} to remove the popup from the stand-alone
contribution page.

* bug 1490727: Reformat translations

Use {% trans %} for multi-line strings, and format them to limited
column width. Extract the URLs from the translated strings, so
translators won't mess with them. Use <em> instead of <i>.

* bug 1490727: Move standard import above others

jwhitlock added a commit that referenced this pull request Sep 20, 2018

CTA on homepage (#5)
* Bug 1487656 ‑ Comment out the indicator `:matches(…)` block for now

- https://bugzil.la/1487656

* Bug 1489981, history button goes to edit instead on dashboard

* Bug 1490392: Reduce web font usage (#4967)

Squashes several commits:
* Removing Open Sans from Gaia demos
* Fixing linting issue on mixins
* removing open sans from maintenance page
* Tweaking styles now that Verdana is in place to minimize differences
* Preload zilla bold
* Removing actual font files
* Using a variable in the breakpoints
* Using rems for letter-spacing
* Using body-font-size in document
* Font smoothing

* update sub-modules

* bug 1490727: Add stripe 2.7.0 requirement

The Python library for the Stripe payments API.

* bug 1490727: Update to stripe 2.8.0

* stripe 2.7.0 → 2.8.0: Add support for automatic retries

* bug 1490727: Update to stripe 2.8.1

* stripe 2.8.0 → 2.8.1: Sync features with ruby library

* added CTA on homepage

* added local storage functionality to remove cta

* fix newline files

* add context processor for contribution form

* fix mobile view, link mobile to /contribute

* fix linting

* address internal feedback

* rename cta class

* Fixed popover button interaction.

* Fix contibute banner button display on mobile.

* Fix padding and margins on form button

* Fixed popover styling for contribution form.

* Squash - PR feedback and code organisation

* Updated the height of the popover for actions

* Implemented FAQ styles, merged with current master branch. (#6)

* Implemented FAQ styles, merged with current master branch.

* Fix newline linting issue on SVGs

* added newline in js

* Address PR feedback, fix linting errors.

* Fix some minor design feedback, update classnames

* Include some legal copy and feedback form.

jwhitlock added a commit that referenced this pull request Sep 20, 2018

Implement the rest of the user flow for payments. (#9)
* Bug 1487656 ‑ Comment out the indicator `:matches(…)` block for now

- https://bugzil.la/1487656

* Bug 1489981, history button goes to edit instead on dashboard

* Bug 1490392: Reduce web font usage (#4967)

Squashes several commits:
* Removing Open Sans from Gaia demos
* Fixing linting issue on mixins
* removing open sans from maintenance page
* Tweaking styles now that Verdana is in place to minimize differences
* Preload zilla bold
* Removing actual font files
* Using a variable in the breakpoints
* Using rems for letter-spacing
* Using body-font-size in document
* Font smoothing

* update sub-modules

* bug 1490727: Add stripe 2.7.0 requirement

The Python library for the Stripe payments API.

* bug 1490727: Update to stripe 2.8.0

* stripe 2.7.0 → 2.8.0: Add support for automatic retries

* bug 1490727: Update to stripe 2.8.1

* stripe 2.8.0 → 2.8.1: Sync features with ruby library

* bug 1490727: Fix path to American Express logo

* bug 1490727: Remove duplicate code

These should have been removed when moved to the contributions app.

* bug 1490727: Add locale-prefix to contribute

Make /contribute a locale-prefixed URL, and add tests for both the
enabled and disabled view.

* added CTA on homepage

* added local storage functionality to remove cta

* fix newline files

* add context processor for contribution form

* fix mobile view, link mobile to /contribute

* fix linting

* address internal feedback

* rename cta class

* Fixed popover button interaction.

* Fix contibute banner button display on mobile.

* Fix padding and margins on form button

* Fixed popover styling for contribution form.

* Squash - PR feedback and code organisation

* Updated the height of the popover for actions

* Implemented FAQ styles, merged with current master branch. (#6)

* Implemented FAQ styles, merged with current master branch.

* Fix newline linting issue on SVGs

* added newline in js

* Address PR feedback, fix linting errors.

* Fix some minor design feedback, update classnames

* Include some legal copy and feedback form.

* fix for celery worker in mdn contribution thank you email sending

* CI build failures

* CI whitespace fix

* Addressed design feedback for legal copy

* Updated Copy

* fix for Decimal rounding in ContributionForm

* Call to action design feedback

* Fix grammar on legal copy

* Update for user testing branch (#7)

* bug 1490727: Minimize SVG with svgo

* bug 1490727: Use blocks instead of URLs

Use a Jinja2 {% block %} to remove the popup from the stand-alone
contribution page.

* bug 1490727: Reformat translations

Use {% trans %} for multi-line strings, and format them to limited
column width. Extract the URLs from the translated strings, so
translators won't mess with them. Use <em> instead of <i>.

* bug 1490727: Move standard import above others
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment