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

Remove font loading in GlobalStyle #54

Closed
kylesuss opened this issue Jul 2, 2019 · 12 comments · Fixed by #77
Closed

Remove font loading in GlobalStyle #54

kylesuss opened this issue Jul 2, 2019 · 12 comments · Fixed by #77
Labels

Comments

@kylesuss
Copy link
Collaborator

kylesuss commented Jul 2, 2019

Remove the font loading of Nunito Sans here or potentially configure the GlobalStyle to conditionally load it.

Currently, design system consumer app frameworks could re-render layout components (Gatsby is an example of a framework that does this) on each page change. This causes the font to be requested on every page change and also a flickering of fonts when that happens.

It might be easiest to just pass a prop to GlobalStyle like this:

export const GlobalStyle = createGlobalStyle`
  ${props => !props.withoutFonts && `
    @import url('https://fonts.googleapis.com/css?family=Nunito+Sans:400,700,800,900');
  `

  body {
    ${bodyStyles}
    margin: 0;
    overflow-y: auto;
    overflow-x: hidden;
  }
`;

// Should we assume that font loading is the default?
<GlobalStyle />
<GlobalStyle withoutFonts />

...but other ideas are welcomed as well.

@qoalu
Copy link

qoalu commented Jul 4, 2019

Conditional loading wouldn’t fix the problem of reloading and flicker (due to rerender), right?

@kylesuss
Copy link
Collaborator Author

kylesuss commented Jul 4, 2019

@qoalu no def not. My thought process was that the app consuming the design system would have to then setup their font loading manually in a way that made sense for their dev environment/framework/library. This was just a way to give them the opportunity to do that.

@qoalu
Copy link

qoalu commented Jul 4, 2019

I was contemplating a similar approach in our design system. Would be interested to see what other options people come up with.

Another idea is to make a side-effect, outside the react lifecycle

@kylesuss
Copy link
Collaborator Author

kylesuss commented Jul 4, 2019

Nice. What do you mean by side-effect?

@qoalu
Copy link

qoalu commented Jul 4, 2019

Something like useEffect and creating a node with a certain id in the dom (using portal maybe)
Keeping track of it not to post the same code twice.
Emotion 9 has an injectGlobalStyle that just hard pasted code into html, actually

@kristofdombi
Copy link

kristofdombi commented Aug 6, 2019

I did some research 🧐on your problem and I've found the following:

There was a long discussion about the same problem you experience at styled-components.

The gist of it is the following:

  • createGlobalStyle won't have any future release, which is going to solve this problem, as they've found the solution is out of styled-components context, the maintainers locked this issue

Possible solutions were suggested:

  • Tweak the font files' caching headers, so the browser won't re-request them. (I suppose this is not an option for you, as you have no control over the caching headers in this case.)
  • Have a .css file only for @font-face and @import rules and make it part of your bundle.

Hope I could help a bit. 🙂

@kylesuss
Copy link
Collaborator Author

kylesuss commented Aug 6, 2019

Nice summary @kristof0425 . I wonder if this means that we should just avoid loading the font in our design system alltogether and let the client applications each handle it individually. Or is there benefit to conditionally loading it as a prop as mentioned here:

#54 (comment)

Anyone have opinions?

@kristofdombi
Copy link

I looked at antd and baseui and I didn't find any @import or @font-face rules within the source files.

IMHO, I would avoid loading the font and let it be handled by the client. I don't see the benefit of loading it conditionally as the re-requesting problem would still persist when the condition is met.

@tmeasday
Copy link
Member

tmeasday commented Sep 11, 2019

Just an bit of extra colour here -- https://github.com/tmeasday/next-safari-reproduction/blob/master/README.md is a repo that reprouduces a problem we've seen due to the use of this @import in combination with NextJS SSR + the styled-components global style in Safari.

This is quite a nasty issue affecting our production app (occassionally server rendered pages just appear basically empty), and I'd say a pretty good reason we shouldn't recommend anyone uses the @import this way (of course if they aren't using SSR they are probably fine, but still).

tmeasday added a commit that referenced this issue Sep 11, 2019
This isn't a complete fix for #54, but it is a step forward, as it allows users to safely do:

```js
import { createGlobalStyle } from 'styled-components';
import { global } from '@storybook/design-system';

const CustomGlobalStyle = createGlobalStyle`
   body {
     ${global.bodyStyles}
   }
}`
```

In their own code to avoid the `@import`.
@tmeasday
Copy link
Member

As a simple first step I did this: #76

I was thinking about adding the withoutFonts prop as suggested in this issue but I feel like we need a decision about whether we actually ever want to support "with fonts". Right now I feel like the answer is no.

@kylesuss
Copy link
Collaborator Author

Agreed @tmeasday . See #77 which handles removing the import.

@kylesuss
Copy link
Collaborator Author

🚀 Issue was released in v1.0.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants