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 nunito sans font #20846

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Add nunito sans font #20846

merged 5 commits into from
Feb 2, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jan 31, 2023

Resolve: #20828
Resolve: #16278

What I did

  • I add a few font files
  • I add these files to be hosted / copied by storybook via a preset config
  • I add references to these font files in the manager builder's template

How to test

  • start a storybook in dev-mode
  • open the browser
  • expect the network tab to show you the browser loading 2 font files in the manager, followed by a css file.
  • expect the font defined in that css files to be used in the manager

repeat the above steps but for a build storybook. (you can use a chromatic deployed storybook for this)

@ndelangen ndelangen changed the title Valentin/add-nunito-sans Add nunito sans font to the manager builder's template Jan 31, 2023
@yannbf
Copy link
Member

yannbf commented Jan 31, 2023

Interesting (and understandable) that Chromatic didn't pick up visual changes, but here are a few I noticed:

Sidebar

2023-01-31 12 11 48


Toolbar and controls

2023-01-31 12 14 37

@ndelangen
Copy link
Member Author

I was instructed to use "Regular" and "Black" variation, thus that's the fonts-files I added.

@MichaelArestad can you please comment?

@MichaelArestad
Copy link
Contributor

I updated the typefaces to include Regular, Italic, Bold, and Bold Italic. I removed black and updated the few places the 900 weight was used.

  1. Is there concern in removing the black option from the theming package?
  2. The italic weight would only be used on docs pages. Is it possible to conditionally load/preload the italic typefaces only for docs pages? Or perhaps load them after everything else on non-docs pages?

Copy link
Member Author

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

nice!

@ndelangen
Copy link
Member Author

Is there concern in removing the black option from the theming package?

No, I have no concerns about that at all.


The italic weight would only be used on docs pages. Is it possible to conditionally load/preload the italic typefaces only for docs pages? Or perhaps load them after everything else on non-docs pages?

This PR doesn't impact the preview (where docs renders) at all.
If the user doesn't have nunito sans installed, they'll still see the fallback font in the preview, even after merging this PR.

Am I correct in assuming you want me to apply this font-loading change to for the preview as well? @MichaelArestad

@socket-security
Copy link

socket-security bot commented Feb 1, 2023

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected malware ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

@MichaelArestad
Copy link
Contributor

Am I correct in assuming you want me to apply this font-loading change to for the preview as well? @MichaelArestad

@ndelangen Yes, please. The only fonts the manager use are regular and bold. Only the docs pages would utilize the italic variants.

@ndelangen ndelangen changed the title Add nunito sans font to the manager builder's template Add nunito sans font Feb 2, 2023
@ndelangen ndelangen requested a review from yannbf February 2, 2023 10:58
@valentinpalkovic
Copy link
Contributor

@ndelangen Thank you, Norbert, for taking care of the requested changes. Should we merge? :)

@ndelangen ndelangen merged commit 505a5af into next Feb 2, 2023
@ndelangen ndelangen deleted the valentin/add-nunito-sans branch February 2, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the Nunito Sans typeface to Storybook Proposal to update the fonts in Storybook
4 participants