fix: update unicode ranges for IBM plex#21748
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21748 +/- ##
=======================================
Coverage 94.90% 94.90%
=======================================
Files 541 541
Lines 45250 45250
Branches 6331 6386 +55
=======================================
Hits 42946 42946
Misses 2172 2172
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
adamalston
left a comment
There was a problem hiding this comment.
Could coverage be added for these changes in https://github.com/carbon-design-system/carbon/blob/f044bf02a661c0e2d2af07716541dbd3b8091887/packages/styles/scss/fonts/__tests__/fonts-test.js or is it not possible? It appears that the current font tests only assert src, font-style, and font-weight.
carbon/packages/styles/scss/fonts/__tests__/fonts-test.js
Lines 62 to 74 in f044bf0
|
@adamalston @emyarod Let me know if there's anything still needed before merging! |
|
I re-scanned the changes and didn't see anything. |
|
It's super cool to see that chromatic caught the glyph changes 🔥 |
597001a
* fix: update unicode ranges for IBM plex * chore: update @ibm/plex * chore: run dedupe * fix(fonts/unicode): correct sass parameter type * fix(fonts/unicode): ensure backwards compatibility * fix: fix * chore(fonts/unicode): fix copyright headers * docs(fonts/unicode): clarify deprecation date * fix(fonts/unicode): update serif ranges * test(styles): add unicode-range to fonts snapshot * chore: run dedupe * chore: revert unintended dependency changes * chore: run dedupe * chore: fix copyright header --------- Co-authored-by: emyarod <emyarod@users.noreply.github.com>
Currently, the advertised unicode ranges for IBM Plex in the
@carbon/stylespackage are outdated. In addition, the same ranges are used across sans, serif and mono fonts even though the respective fonts differ in support.The concrete issue I ran into was that the unicode border glyphs weren't properly rendering using Plex Mono as the respective range
U+2500-259Fwasn't advertised in the@font-face. OnlyU+25CAwas listed, as this is the only one supported by sans.I split up the unicode range constants per type (sans, serif, mono) and updated the lists based on their current state in
ibm/plex:Changelog
Changed
/packages/styles/scss/fonts/unicodeinto separate files:_sans.scss_serif.scss_mono.scssget-range($name))@ibm/plexpackage from an outdated pre-release (6.0.0-next.6) to the latest6.xversion.Testing / Reviewing
main, build thestylespackage and inspect the generated/packages/styles/css/styles.css.U+2500. It should yield no matches.stylespackaged. Inspect the generated CSS again.U+2500should yield results for all mono "-Pi" fonts, but not for sans and serif. Expected range:U+2500-259F<code>…</code>element via the browser dev tools:On main (or on the deployed storybook), it should look broken:
On the PR storybook, it should be rendered correctly:
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
Updated documentation and storybook examplesWrote passing tests that cover this changeAddressed any impact on accessibility (a11y)More details can be found in the pull request guide