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(styles): add support for IBM Plex from the @ibm/plex package #10205

Merged

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Dec 1, 2021

This PR adds in support for bringing in IBM Plex to Carbon through scss/fonts. This update allows teams to bundle the latest version of @ibm/plex into their project, in particular for bundlers like webpack. It also allows teams that self-host to use our system to correctly determine url() values for @font-face blocks.

Overall, this PR by default allows IBM Plex Mono, Sans, Serif to be included with the light, regular, and semibold font weights. It also allows teams to include other font weights and other font families such as:

  • IBM Plex Sans Arabic
  • IBM Plex Sans Devanagari
  • IBM Plex Sans Hebrew
  • IBM Plex Sans Thai
  • IBM Plex Sans Thai Looped

This PR does not include the following:

  • IBM Plex Sans JP
  • IBM Plex Sans KR

Changelog

New

  • Add scss/fonts entrypoint to @carbon/styles

Changed

  • Update how fonts are brought in with @carbon/styles
  • Update how storybook brings in fonts in carbon-react

Removed

  • Removed scss/_font-face.scss

Testing / Reviewing

  • packages/styles/scss/fonts
    • Review the implementation and see if it makes sense to you
    • Make sure implementation is tested and the tests pass
    • Review the scss/fonts/README.md file and make content suggestions where appropriate if something is unclear or missing from the docs
  • packages/carbon-react
    • Verify that all files from packages/styles/scss/fonts are correctly re-exported in packages/carbon-react/scss/fonts
    • In the storybook, visit the Elements / IBM Plex story and do the following:
      • Verify each font loads as a network resource
      • Verify fonts with unicode ranges only load the relevant unicode range
      • There's a video below of what this would look like
Screen.Recording.2021-12-01.at.14.08.55.mov

At the end, you'll see that I noticed that IBM Plex Sans Thai was loaded incorrectly and didn't not have the correct font resource loaded

@joshblack joshblack requested a review from a team as a code owner December 1, 2021 20:12
@joshblack joshblack changed the title Feat/add support for fonts feat(styles): add support for IBM Plex from the @ibm/plex package Dec 1, 2021
@joshblack joshblack requested review from abbeyhrt and dakahn December 1, 2021 20:13
@joshblack
Copy link
Contributor Author

FYI @vpicone

@netlify
Copy link

netlify bot commented Dec 1, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: ea40851

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61aa48a12b52a30007337816

😎 Browse the preview: https://deploy-preview-10205--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Dec 1, 2021

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: ea40851

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61aa48a111d27e00070bda13

😎 Browse the preview: https://deploy-preview-10205--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Dec 1, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: ea40851

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61aa48a10298910008e6e56a

😎 Browse the preview: https://deploy-preview-10205--carbon-elements.netlify.app

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

This is really great work, I'm super impressed! Huge value in this one.

Just a couple questions below but ultimately LGTM 👍

packages/styles/scss/fonts/README.md Show resolved Hide resolved
packages/styles/scss/fonts/README.md Show resolved Hide resolved
Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

looks great josh! all tests pass and the implemention is really clean. Thanks for going over this earlier

);
```

You can also configure it to disable specific fonts:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you disable a font here do you need to enable another one by default or will the other 2 default ones just take precedent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aledavila great question, basically there are the three fonts that are enabled by default. If you disable one of them, the others will still be enabled.

The trick for this is in the fonts/_index.scss file that merges our "default" fonts with the given $fonts map:

$-default-fonts: (
  IBM-Plex-Sans: true,
  IBM-Plex-Serif: true,
);

// This is the variable that callers would change with `@use`
$fonts: () !default;

// Merge the two maps, prefer values in what the user gives us over our default values
$fonts: map.merge($-default-fonts, $fonts);

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh cool. Thank !


Each font is available as an entrypoint in the `fonts` folder. You can use these
entrypoints to include specific font weights, styles, and more for IBM Plex. For
example, if you only want to include the regular font weight for IBM Plex Sans
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we surface the available font weights here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, definitely 👍

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks great! Very cool to see, I've never thought about fonts before and it's awesome to officially have this in carbon now!

@kodiakhq kodiakhq bot merged commit 162a5a8 into carbon-design-system:main Dec 3, 2021
@joshblack joshblack mentioned this pull request Dec 3, 2021
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.

4 participants