refactor(react-icons): generate font-family definitions from one manifest - #1218
Draft
Martin Hochel (Hotell) wants to merge 1 commit into
Draft
Conversation
📋 PR Validation SummaryCheck the Build react library job summary for detailed reports:
|
…fest The CSS font-family, the font binary names, the `data-fui-icon-font` value and the `FontFile` enum value were duplicated across six hand-maintained places, which is how the Light family ended up declared on the headless surface but emitted as `font-family: undefined` on the Griffel one. `scripts/font-families.js` is now the single source of truth, and `scripts/generate-font-styles.js` (wired into `build:fonts-and-svg`) rewrites the derived `#region @generated *` blocks in place — the surrounding code stays hand-written, type-checked and committed, and `--check` fails on a stale checkout. `copy-base-fonts.js` and `convert-font*.js` consume the same manifest instead of their own family lists and hardcoded `0/1/2/3` style codes. Adds a build-verify assertion that the shipped Griffel and headless artifacts declare exactly the same families, plus unit tests pinning the enum values that are baked into the generated atoms.
Martin Hochel (Hotell)
force-pushed
the
feat/react-icons-generate-font-definitions
branch
from
August 21, 2026 14:08
99e8a55 to
560bd93
Compare
| * @param {string} key | ||
| * @returns {FontFamilyDefinition} | ||
| */ | ||
| function getFontFamilyByKey(key) { |
Collaborator
Author
There was a problem hiding this comment.
this should be string union rather string
| family: 'FluentSystemIcons', | ||
| variant: 'resizable', | ||
| file: 'FluentSystemIcons-Resizable', | ||
| resizable: true, |
Collaborator
Author
There was a problem hiding this comment.
what is this resizable about ?
There was a problem hiding this comment.
Pull request overview
Centralizes React icon font metadata in one manifest and generates consistent Griffel/headless definitions.
Changes:
- Adds the font-family manifest and region generator.
- Updates font conversion/copying to consume manifest data.
- Adds source and shipped-artifact consistency tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/react-icons/src/utils/fonts/createFluentFontIcon.styles.ts |
Adds generated Griffel font definitions. |
packages/react-icons/src/utils/fonts/createFluentFontIcon.shared.ts |
Marks FontFile as generated. |
packages/react-icons/src/headless/styles.css |
Marks headless family rules as generated. |
packages/react-icons/src/headless/fonts/styles.css |
Generates headless font faces. |
packages/react-icons/src/headless/fonts/createFluentFontIcon.tsx |
Generates variant mappings. |
packages/react-icons/scripts/generate-font-styles.test.js |
Tests manifest and generated regions. |
packages/react-icons/scripts/generate-font-styles.js |
Implements region generation. |
packages/react-icons/scripts/font-families.js |
Defines the central manifest. |
packages/react-icons/scripts/copy-base-fonts.js |
Copies manifest-defined fonts. |
packages/react-icons/scripts/convert-font.utils.js |
Uses manifest enum values. |
packages/react-icons/scripts/convert-font.js |
Resolves font files through the manifest. |
packages/react-icons/package.json |
Integrates generation into builds. |
packages/react-icons/build-verify.test.js |
Verifies generated package consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "generate:font-styles": "node scripts/generate-font-styles.js --dest=./src", | ||
| "generate:rtl": "node ../../importer/rtlMetadata.js --source=../../assets --dest=./intermediate/rtl.json", | ||
| "build:fonts-and-svg": "yarn generate:base-svg && yarn generate:font && yarn generate:rtl", | ||
| "build:fonts-and-svg": "yarn generate:base-svg && yarn generate:font && yarn generate:font-styles && yarn generate:rtl", |
|
|
||
| // Check for different font types | ||
| const fontTypes = ['Regular', 'Filled', 'Light', 'Resizable']; | ||
| const fontTypes = FONT_FAMILIES.map(({ key }) => key); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on #1217 — the first commit here is that fix. Review the second commit, or merge #1217 first and this diff shrinks to it.
Why
The CSS
font-family, the font binary base name, thedata-fui-icon-fontvalue and theFontFileenum value were duplicated across six hand-maintained places:src/utils/fonts/createFluentFontIcon.shared.tsFontFileenum valuessrc/utils/fonts/createFluentFontIcon.styles.ts@font-faceblock,useRootStylesvariantssrc/headless/fonts/styles.css@font-faceblocksrc/headless/styles.css[data-fui-icon-font='…']rulessrc/headless/fonts/createFluentFontIcon.tsxFONT_VARIANT_MAPscripts/copy-base-fonts.js+scripts/convert-font.utils.js0/1/2/3style codesThat is exactly how the Light family ended up declared on the headless surface but emitted as
font-family: undefinedon the Griffel one (#1217) — nothing failed the build, the icons just rendered with no font.What
scripts/font-families.jsis the single source of truth (key/ enumvalue/family/variant/file/resizable).scripts/generate-font-styles.jsrewrites the derived#region @generated <name>blocks in place. The surrounding code stays hand-written, committed and type-checked — only the derived data is generated, so the prose comments (theoverflow: hiddenbaseline note, the:where()specificity note) survive and the files remain reviewable in a normal diff.--checkfails instead of writing, for a stale checkout.yarn generate:font-styles, part ofbuild:fonts-and-svg.copy-base-fonts.jsandconvert-font{,.utils}.jsnow consume the manifest instead of their own family lists and magic style codes.Guardrails
scripts/generate-font-styles.test.js— pins theFontFilevalues (they are baked into ~24k generated atoms), asserts unique families/variants/files, asserts every@font-facehas afont-display, and asserts the checked-in sources match the generated regions.build-verify.test.js— new assertion that the shipped artifacts agree: the families declared inlib/utils/fonts/createFluentFontIcon.styles.jsand inlib/headless/{fonts/,}styles.cssare identical and match the manifest. This is the regression test for fix(react-icons): render Light font icons #1217.Notes for review
@font-facerules are now emitted in manifest order (Filled, Regular, Resizable, Light); Light used to sit third. Distinct families, so no cascade impact — it is visible in the snapshot diff.convert-font.jskeeps an explicitSIZED_ORDER = ['Light', 'Filled', 'Regular']: that order is load-bearing for chunk assignment, so it is deliberately not derived from the manifest.makeStaticStyles(...), so the generated region spans the whole statement.Verification
yarn nx run react-icons:build+build-verify— 140 passed / 8 skipped,metadata.jsonunchanged (no chunk churn).yarn test— 112 passed.lint,type-check:infra, prettier clean.