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

[Bug]: @storybook/nextjs not loading google fonts with 3 words in name #22248

Closed
msadura opened this issue Apr 25, 2023 · 4 comments · Fixed by #23121
Closed

[Bug]: @storybook/nextjs not loading google fonts with 3 words in name #22248

msadura opened this issue Apr 25, 2023 · 4 comments · Fixed by #23121

Comments

@msadura
Copy link

msadura commented Apr 25, 2023

Describe the bug

Google font is not loaded correctly when it has more than 2 words (parts) in name. Loaded font's className and variable have space in name and do not work probably because of that.

import font code:

import { PT_Sans_Narrow } from 'next/font/google';

export const font = PT_Sans_Narrow({
  subsets: ['latin'],
  variable: '--font-family-sans',
  fallback: ['Georgia', 'ui-serif', 'serif'],
  weight: ['400', '700']
});

output:

{
    "className": "pt-sans narrow-normal",
    "style": {
        "fontFamily": "PT Sans Narrow",
        "fontStyle": "normal"
    },
    "variable": "__variable_pt-sans narrow-normal"
}

Fonts with 1 or 2-part names works, looks like 3rd part gets space instead of - in className

To Reproduce

https://github.com/msadura/sb-google-font-bug-repro/tree/main/nextjs/default-ts

start sotrybook and check console log from preview.ts - google font className and variable are incorrect.

System

System:
    OS: macOS 13.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.12.1 - ~/.asdf/installs/nodejs/18.12.1/bin/node
    Yarn: 1.22.19 - ~/node_modules/.bin/yarn
    npm: 8.19.2 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Chrome: 112.0.5615.137
    Firefox: 112.0.1
    Safari: 16.2
  npmPackages:
    @storybook/addon-essentials: ^7.0.6 => 7.0.6 
    @storybook/addon-interactions: ^7.0.6 => 7.0.6 
    @storybook/addon-links: ^7.0.6 => 7.0.6 
    @storybook/blocks: ^7.0.6 => 7.0.6 
    @storybook/nextjs: ^7.0.6 => 7.0.6 
    @storybook/react: ^7.0.6 => 7.0.6 
    @storybook/testing-library: ^0.0.14-next.2 => 0.0.14-next.2 

nextjs version: 13.2, 13.3.1

Additional context

No response

@u840903
Copy link

u840903 commented May 30, 2023

Just spent two hours on figuring out why IBM Plex Sans didn't work.
https://fonts.google.com/specimen/IBM+Plex+Sans

@shilman
Copy link
Member

shilman commented May 31, 2023

Cc @valentinpalkovic

@ygkn
Copy link
Contributor

ygkn commented Jun 16, 2023

Hi, I have found workarounds and why this issue causes.

Workaround

Although the class name has spaces and doesn't work, the font itself still be loaded. So you can directly refer to the font-family in CSS.

Here's an example using decorator:

  decorators: [
    (story) => (
      <>
        <style>
          {`
			/* classNames method */
            .${font.className.replaceAll(' ', '.')} {
              font-family: ${font.style.fontFamily};
              ${font.style.fontStyle ? `font-style: ${font.style.fontStyle};` : ''}
              ${font.style.fontWeight ? `font-weight: ${font.style.fontWeight};` : ''}
            }

            ${
              font.variable
                ? `
				  /* CSS variable method */
                  .__variable_${font.className.replaceAll(' ', '.')} {
                    --font-family-sans: '${font.style.fontFamily}';
                    /* ^ Note that this is the 'variable' option you set */
                  }
                  `
                : ''
            }
          `}
        </style>
        {story()}
      </>
    ),
  ],

The styles method works as it is.

Why

The issue originates from the getClassName function in get-css-meta.ts.

const font = fontFamily.replace(' ', '-').toLowerCase();

The replace method only replaces a single space, causing the problem.

How to fix

To fix the bug, we can replace replace with either replaceAll (I prefer this) or replace(/ /g, '-') to replace all spaces.

Would it be alright if I submit a pull request with the fix?
Thank you.

@ygkn
Copy link
Contributor

ygkn commented Jun 18, 2023

I read Code contributions document and opened PR #23121.

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

Successfully merging a pull request may close this issue.

4 participants