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

feat: Next 13 global fonts #2313

Merged
merged 5 commits into from
May 16, 2024
Merged

feat: Next 13 global fonts #2313

merged 5 commits into from
May 16, 2024

Conversation

lucasfp13
Copy link
Contributor

What's the purpose of this pull request?

This PR intends to introduce next/font built-in feature to the project (app directory).

The babelrc.js config file needed to be removed in favor of next/font usage, it's mandatory. Next.js requires the use of its own compiler (SWC) config, so for now we still don't have the necessary GraphQL Codegen plugin to replace the @graphql-codegen/client-preset being loaded in the Babel config file, as the existing one (@graphql-codegen/client-preset-swc-plugin) has a bug that hasn't been solved yet (see here and here.

Also, right now we are just introducing the feature, but we will have to support local fonts and font override later in other tasks.

How to test it?

  • Run the local server (yarn turbo run dev --filter=@faststore/core);
  • Make sure that all the current pages are working as expected (/, /office and some /<slug>/p);
  • Make sure /fs-next-update is rendering with some text on the top of the page and without errors.

References

@lucasfp13 lucasfp13 self-assigned this May 13, 2024
@lucasfp13 lucasfp13 requested a review from a team as a code owner May 13, 2024 22:09
@lucasfp13 lucasfp13 requested review from eduardoformiga and lariciamota and removed request for a team May 13, 2024 22:09
Copy link

vercel bot commented May 13, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
faststore-site ⬜️ Ignored (Inspect) Visit Preview May 15, 2024 9:09pm

@lucasfp13 lucasfp13 changed the title feat: /sfs 763 use next font feat: Next 13 fonts May 13, 2024
@lucasfp13 lucasfp13 changed the title feat: Next 13 fonts feat: Next 13 global fonts May 13, 2024
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

good job! I left some comments along with the review.

I think we should create new tasks in our backlog to deal in the future with overriding fonts, also 1 documentation task with the recommended way to use fonts (new way), maybe indicating to use something like:

--fs-text-face-body: var(--fs-font-inter)

or

font-family: var(--fs-font-inter)

As we change the swc/babel/plugin configs. It would also be good to test the API extension with the version of this PR (I will do that).

packages/core/.babelrc.js Show resolved Hide resolved
packages/core/app/layout.tsx Outdated Show resolved Hide resolved
Comment on lines 85 to 86
* Later when overriding fonts we should use the font variable in CSS files
* https://nextjs.org/docs/app/api-reference/components/font#css-variables
Copy link
Member

Choose a reason for hiding this comment

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

💯

packages/core/app/layout.tsx Show resolved Hide resolved
packages/core/app/styles/fonts/index.tsx Show resolved Hide resolved
Copy link

codesandbox-ci bot commented May 15, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

I tested it with the API extension + section overrides v2 and it works as expected.

faststore core version from

starter pr

commit with the version
vtex-sites/starter.store@a68d962

@lucasfp13 lucasfp13 merged commit 1ca0ce0 into main May 16, 2024
7 checks passed
@lucasfp13 lucasfp13 deleted the feat/SFS-763-use-next-font branch May 16, 2024 13:06
eduardoformiga added a commit that referenced this pull request Jun 12, 2024
eduardoformiga added a commit that referenced this pull request Jun 13, 2024
This PR intends to introduce `next/font` built-in feature to the project
(`app` directory).

The `babelrc.js` config file needed to be removed in favor of
`next/font` usage, [it's
mandatory](https://nextjs.org/docs/messages/babel-font-loader-conflict).
Next.js requires the use of its own compiler (SWC) config, so for now we
still don't have the necessary GraphQL Codegen plugin to replace the
`@graphql-codegen/client-preset` being loaded in the Babel config file,
as the existing one
([`@graphql-codegen/client-preset-swc-plugin`](https://the-guild.dev/graphql/codegen/plugins/presets/preset-client#swc-plugin))
has a bug that hasn't been solved yet (see
[here](dotansimha/graphql-code-generator#9753)
and
[here](dotansimha/graphql-code-generator#9450).

Also, right now we are just introducing the feature, but we will have to
support local fonts and font override later in other tasks.

- Run the local server (`yarn turbo run dev --filter=@faststore/core`);
- Make sure that all the current pages are working as expected (`/`,
`/office` and some `/<slug>/p`);
- Make sure `/fs-next-update` is rendering with some text on the top of
the page and without errors.

- https://nextjs.org/blog/next-13#nextfont
-
https://nextjs.org/docs/app/api-reference/components/font#css-variables
- https://nextjs.org/docs/app/building-your-application/optimizing/fonts
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