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

Add fontfaces and the use-font-face-observer hook #297

Merged
merged 10 commits into from
Aug 10, 2022

Conversation

taylrj
Copy link
Contributor

@taylrj taylrj commented Aug 3, 2022

Address TWREPORTER-318

@taylrj taylrj requested a review from duidae August 3, 2022 04:14
@taylrj taylrj self-assigned this Aug 3, 2022
@vercel
Copy link

vercel bot commented Aug 3, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-components ✅ Ready (Inspect) Visit Preview Aug 10, 2022 at 0:48AM (UTC)

@taylrj taylrj changed the title Add fontfaces and their observer hook Add fontfaces and the use-font-observer hook Aug 3, 2022
@taylrj taylrj changed the title Add fontfaces and the use-font-observer hook Add fontfaces and the use-font-face-observer hook Aug 3, 2022
Copy link
Contributor

@duidae duidae left a comment

Choose a reason for hiding this comment

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

Mostly looking good, and nice to have extensibility for loading fonts

有個小問題想確認:
1. 如果Noto Sans TC載入失敗, 是不是字型顯示就會轉為browser default?

@taylrj
Copy link
Contributor Author

taylrj commented Aug 4, 2022

有個小問題想確認:

  1. 如果Noto Sans TC載入失敗, 是不是字型顯示就會轉為browser default?

Nice Catch!
是,但應該要提供符合 guideline 的 fallback。

04a9e21 address this issue.

@duidae
Copy link
Contributor

duidae commented Aug 5, 2022

@taylrj 感謝解答

另外想請問一個問題,就是為什麼self host font載入是選擇用fontfaceobserver-es這個loader而不是直接用<link rel="preload"...>

ref: https://web.dev/preload-optional-fonts/
https://web.dev/codelab-preload-web-fonts/

主要想法是參考Martin Fowler的Evolutionary Design(開發時組件的功能如果有用到再加入),上面這些eslint的warning, 尚未使用到的state isResolved主要源自使用hook這個設計,所以想說如果字型載入這件事用<link rel="preload"...>或放到app.js的constructor能否簡化一點


preload解決的是FOIT, fontfaceobserver-es利用觀察fontface(font-display: swap)來執行callback換css,解決FOUT

@taylrj
Copy link
Contributor Author

taylrj commented Aug 5, 2022

After the discussion, a patch will be added to twreporter/twreporter-react#2216 for preloading the self hosted font files.

@taylrj
Copy link
Contributor Author

taylrj commented Aug 5, 2022

After the discussion, a patch will be added to twreporter/twreporter-react#2216 for preloading the self hosted font files.

Updated.

@duidae
Copy link
Contributor

duidae commented Aug 8, 2022

LGTM, great integration of self-host fonts, thanks!

taylrj added 6 commits August 10, 2022 08:44
This change adds get-fontfaces utility function to get the fontfaces by
font-family and font-weight specified in @twreporter/core.
This change adds `fontobserver-es` npm module monitor when a webfont is loaded and notify.
Please note that it is different from `fontfaceobserver` module, we
choose `fontobserver-es` because it's an es module.

ref:
- bramstein/fontfaceobserver#118
- https://github.com/dmnsgn/fontfaceobserver
This change adds the `use-font-face-observer` react hook which utilize
fontfaceobserver and returns whether the specified fonts are loaded then
run callback.
This change removes font-family related css setting in article-page
component and let it inherit the default font-family from
`@twreporter/core` setting directly from `html`.

The reason to this change would be lately add classname to html after
fonts are loaded to prevent FOUT(Flash of Unstyled Text) word by word.
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.

2 participants