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

FLUID-6499: added font display swap for font faces #991

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TeddTech
Copy link
Contributor

Description

Leverages the font-display CSS feature to ensure text is user-visible while webfonts are loading.

Steps to test

  1. Go to a webpage implementing UIO
  2. inspect the page
  3. go to audit tab and run lighthouse

Expected behavior:

Ensure text remains visible during webfont load should no longer come up under Diagnostics because it has already been applied.

Additional information

N/A

Related issues

https://issues.fluidproject.org/browse/FLUID-6499

@idrc-cms-bot
Copy link

Could one of the admins verify that these changes are reasonable to test? If so, please reply with "ok to test".

@cindyli
Copy link
Member

cindyli commented May 15, 2020

ok to test

@TeddTech TeddTech changed the title fix: added font display swap for font faces FLUID-6499: added font display swap for font faces May 15, 2020
@cindyli
Copy link
Member

cindyli commented May 15, 2020

@TeddTech, thanks for the pull request.

All commit messages into the infusion repository also need to be prefixed with the jira number in the format such as "FLUID-6499: add ...", like what you've done for the pull request title.

Please change commit messages too. Thanks.

@idrc-cms-bot
Copy link

@idrc-cms-bot
Copy link

@TeddTech
Copy link
Contributor Author

@TeddTech, thanks for the pull request.

All commit messages into the infusion repository also need to be prefixed with the jira number in the format such as "FLUID-6499: add ...", like what you've done for the pull request title.

Please change commit messages too. Thanks.

will do

@idrc-cms-bot
Copy link

@idrc-cms-bot
Copy link

@sachin10101998
Copy link

sachin10101998 commented May 15, 2020

@TeddTech, there's something terribly wrong with the commits in your PR. The commit
7138650 to 19c8943 have already been merged to master and they should not be present here. This is probably because you didn't sync your fork with the master before sending a PR or the head was not correctly set before you sent PR. You can't edit commits after a PR has been sent, but you can keep a check on commits by using GUI clients like smartGit.

@TeddTech
Copy link
Contributor Author

@TeddTech, there's something terribly wrong with the commits in your PR. The commit
7138650 to 19c8943 have already been merged to master and they should not be present here. This is probably because you didn't sync your fork with the master before sending a PR or the head was not correctly set before you sent PR. You can't edit commits after a PR has been sent, but you can keep a check on commits by using GUI clients like smartGit.

I did a rebase removing the duplicate commits and only keeping my own

@idrc-cms-bot
Copy link

Copy link

@sachin10101998 sachin10101998 left a comment

Choose a reason for hiding this comment

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

Check review below

@@ -1,6 +1,7 @@
@font-face {
font-family:'Orator-Icons';
src:url("../fonts/Orator-Icons.woff")
src:url("../fonts/Orator-Icons.woff");
font-display: swap

Choose a reason for hiding this comment

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

Add ';' here.

@idrc-cms-bot
Copy link

@jobara
Copy link
Member

jobara commented May 19, 2020

@TeddTech could you please update your last commit message to be in the correct format, similar to your previous commits.

@jobara
Copy link
Member

jobara commented May 19, 2020

@TeddTech also, it's best practice to file a PR from a working branch, rather than using your master branch. For example if you needed to file another PR against the repository, you may accidentally include changes from this PR because it's from your master, and it's harder to keep things clean in general.

@idrc-cms-bot
Copy link

@TeddTech
Copy link
Contributor Author

@TeddTech also, it's best practice to file a PR from a working branch, rather than using your master branch. For example if you needed to file another PR against the repository, you may accidentally include changes from this PR because it's from your master, and it's harder to keep things clean in general.

Thanks for the info @jobara. Will do from now on

@TeddTech TeddTech closed this May 19, 2020
@TeddTech TeddTech reopened this May 19, 2020
@jobara
Copy link
Member

jobara commented May 19, 2020

@TeddTech and @cindyli I left a comment on the JIRA.

@TeddTech
Copy link
Contributor Author

The purpose of this PR is to improve performance of websites that integrate infusion. At todays We Count check-in it was decided that leaving font-display as its default value (auto) would be best.

The value for PrefsFramework-Icons.woff in the src/framework/preferences/css/stylus/PrefsEditor.styl file may still need to change because the lighthouse performance test throws a Ensure text remains visible during webfont load error for this font.

@cindyli
Copy link
Member

cindyli commented May 22, 2020

The purpose of this PR is to improve performance of websites that integrate infusion. At todays We Count check-in it was decided that leaving font-display as its default value (auto) would be best.

The value for PrefsFramework-Icons.woff in the src/framework/preferences/css/stylus/PrefsEditor.styl file may still need to change because the lighthouse performance test throws a Ensure text remains visible during webfont load error for this font.

Just to add a couple of clarifications about outcomes from today's WeCount check in:

  1. For font faces defined for font icons, using the browser font-display: auto; is preferable because there aren't fallbacks for them. This means, @TeddTech, please update this pull request to remove font-display: swap; for this category. PrefsFramework-Icons.woff in the src/framework/preferences/css/stylus/PrefsEditor.styl is one of them;

  2. For font faces defined for text fonts, designer's input is needed to decide what user experience is preferred. This pull request will be updated once the decision is made.

@idrc-cms-bot
Copy link

@jobara jobara changed the base branch from master to main October 22, 2020 14:37
@amb26 amb26 added the mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants