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

[Storybook] Preload Inter stylesheet and font in Storybook #10073

Closed
wants to merge 1 commit into from

Conversation

sophschneider
Copy link
Contributor

@sophschneider sophschneider commented Aug 15, 2023

WHY are these changes introduced?

Fixes #10038

Note: I'm hypothesising that properly preloading inter will fix the flakey tests based on the popover/tooltip position investigation I did (see issue), but we will have to see after this is merged. Regardless, its probably best to preload the font

Async loading of the Inter font is causing a discrepancy in chromatic snapshots because of layout shifts caused by the font flashing.

  • My previous attempt to preload the Inter font didn't work (it was just falling back to the SF font)
  • I found this article that suggests to not only preload the stylesheet but to preload the font itself

WHAT is this pull request doing?

Adds Storybook font and stylesheet preloads as per the suggestions from this article

How to 🎩

You can see the font flash visually when doing an Empty cache and hard reload

To replicate the layout shift in storybook:
  1. go to a flakey story LegacyCard with separate header
  2. Empty cache and hard reload
  3. remount component on story to see popover overlay layout shift
  4. compare to fixed story on this PR
replicate-shift.mov
To replicate the font loading bug more explicitly:
  1. put a debugger statement here
  2. go to the LegacyCard with separate header story
  3. Open dev tools
  4. Empty cache and hard reload
  5. Inspect text element
  6. Check the "Rendered fonts" in the computed styles tab
Before on first render After on first render
Screenshot 2023-08-15 at 5 07 15 PM Screenshot 2023-08-15 at 5 08 40 PM

@sophschneider sophschneider marked this pull request as ready for review August 15, 2023 21:29
<link
rel="preload"
as="font"
href="https://fonts.gstatic.com/s/inter/v12/UcC73FwrK3iLTeHuS_fvQtMwCp50KnMa1ZL7W0Q5nw.woff2"
Copy link
Contributor

Choose a reason for hiding this comment

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

This might work today, but given we have no control over how Google hashes its fonts, and these font URLs come from the CSS file which Google is also in control of, the actual font URL we render with could change at any time, therefore breaking the preload.

The solution in #9993 should work for all fonts, even if their URLs change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm maybe we can merge both approaches? I couldn't get #9993 to work locally even with removing the isChromatic and I'm not sure how to test that its working on chromatic itself.

If we merge both maybe having this unstable URL will be less risky

@sophschneider sophschneider marked this pull request as draft August 16, 2023 19:04
@sophschneider
Copy link
Contributor Author

Closing as I proved this still creates flakey tests in #10091

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.

3 participants