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.

Sign up for GitHub

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

Jump to bottom

feat(Plex): add Plex package to carbon-react storybook #8726

Merged
merged 6 commits into from
Jun 2, 2021
Merged

feat(Plex): add Plex package to carbon-react storybook #8726

merged 6 commits into from
Jun 2, 2021

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented May 20, 2021 •
edited
Loading

Closes #8711
Closes #8712

Adds in the @ibm/plex package, as well as enhancing the locale switcher to allow a user to change the locale to Arabic. This can be used to test that the IBM Plex Sans Arabic is properly loaded in.

Changelog

New

  • a Plex story, that shows the 3 font styles currently supported in the @ibm/plex package (Sans, Mono, and Arabic)
  • A fontWeight and fontSize switcher, to test the different weights (and increase font-size for easier testing)

Testing / Reviewing

In the storybook, use the locale switcher to switch between en and ar and ensure the correct fonts are loaded when viewing the Arabic story

@netlify
Copy link

netlify bot commented May 20, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 5294cbe

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

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

@netlify
Copy link

netlify bot commented May 20, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 5294cbe

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

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

@tw15egan tw15egan marked this pull request as ready for review May 20, 2021 20:33
@tw15egan tw15egan requested a review from a team as a code owner May 20, 2021 20:33
@tw15egan tw15egan requested review from joshblack and dakahn May 20, 2021 20:33
joshblack
joshblack suggested changes May 21, 2021
View reviewed changes
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

image

Hmm running into this for rendered fonts, even though the font-family one is working:

image

Am I missing a step potentially? 🤔

@tw15egan
Copy link
Collaborator Author

@joshblack we were seeing that in testing in Chrome, but Safari seemed to be rendering correctly. We'll need to dig deeper, not sure what the issue is

@tw15egan
Copy link
Collaborator Author

@joshblack it seems like it may be caused because the @ibm/plex font files are being installed into the monorepo node_modules folder, and not the @carbon-react, so we may need to configure the storybook config to handle that use-case? I've tried fiddling with file-loader in main.js but not having any luck

@joshblack
Copy link
Contributor

@tw15egan if you add in resolve-url-loader into webpack does that help out? https://www.npmjs.com/package/resolve-url-loader

@tw15egan tw15egan force-pushed the carbon-react-plex branch 2 times, most recently from 66199e3 to 22d26ee Compare May 25, 2021 22:42
@tw15egan tw15egan mentioned this pull request May 25, 2021
Closed
@tw15egan
Copy link
Collaborator Author

@joshblack I've pushed a bunch of fixes and created #8764 to track our font-family stack strategy. This PR demonstrates the ability of the @ibm/plex package to serve as a standalone package / delivery of the Plex font, and should give us a great path forward from moving away from Google Fonts

@tw15egan tw15egan requested review from joshblack and vpicone May 25, 2021 22:46
vpicone
vpicone reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Hmm, looks like maybe font-face declarations might still be getting emitted? Do we want to fiddle with those error messages/feature flag logic in this PR?

Screen Shot 2021-05-25 at 6 36 43 PM

@vpicone
Copy link
Contributor

vpicone commented May 25, 2021

Also I can't find the locale switcher 🤦

@tw15egan
Copy link
Collaborator Author

@vpicone I think we should tackle that deprecation warning in a separate PR on the next branch as we get ready to move forward with using the @ibm/plex package as a dependency. I believe that warning is coming from the components package. I'm only seeing that in the build process, not when running the carbon-react storybook locally. Are you seeing it there?

Also, the locale switcher is the little globe icon at the top:
Screen Shot 2021-05-26 at 10 28 59 AM

@tw15egan tw15egan force-pushed the carbon-react-plex branch from e2acab5 to f86daff Compare May 26, 2021 17:44
@joshblack
Copy link
Contributor

Yeah, I think you were looking at the netlify preview @vpicone which is the current stable storybook. The @carbon/react one doesn't have a deploy preview yet so you need to check it out locally 👍

@vpicone
Copy link
Contributor

vpicone commented May 26, 2021 •
edited
Loading

@tw15egan @joshblack AH NUTS that was it. Thank you.

@kodiakhq
Copy link
Contributor

kodiakhq bot commented Jun 1, 2021

This PR currently has a merge conflict. Please resolve this and then re-add the status: ready to merge 🎉 label.

@tw15egan tw15egan force-pushed the carbon-react-plex branch from f86daff to da97982 Compare June 1, 2021 18:18
@kodiakhq kodiakhq bot merged commit 4f1675a into carbon-design-system:main Jun 2, 2021
emyarod pushed a commit to emyarod/carbon that referenced this pull request Jun 10, 2021
@tw15egan @kodiakhq @emyarod
feat(Plex): add Plex package to carbon-react storybook (carbon-design…
5ac9da5 
…-system#8726)

* feat(Plex): add Plex package to carbon-react storybook

* chore(Plex): update to @ibm/[email protected]

* fix(Plex): update package to fix font loading issue

* fix(Plex Mono): fix Plex Mono issue upstream

* chore(scss): clean up commented code

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vpicone vpicone vpicone left review comments

@joshblack joshblack joshblack approved these changes

@dakahn dakahn dakahn approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bring in IBM Plex font faces into storybook Add locale switcher support to storybook v6 in @carbon/react
4 participants
@tw15egan @joshblack @vpicone @dakahn