-
Notifications
You must be signed in to change notification settings - Fork 764
fix(react-icons): properly resolve atoms grouppings if icon contains style variant as part of its name #955
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
Conversation
2b79dbf to
ca7c5c7
Compare
| * @returns {string} The normalized base name of the icon. | ||
| * @throws {Error} If the filename does not end with a valid style suffix. | ||
| */ | ||
| function normalizeBaseName(fileName, styleVariants = ICON_STYLE_VARIANTS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation was simplified with proper guards to adhere to valid file patterns from /assets provided by design
| const items = [ | ||
| { exportName: 'Dup20Filled', exportCode: 'export const Dup20Filled = 1;', fileName: 'dup-20-filled.tsx' }, | ||
| { exportName: 'Dup20Filled', exportCode: 'export const Dup20Filled = 2;', fileName: 'dup-20-filled-2.tsx' }, | ||
| { exportName: 'Dup20Filled', exportCode: 'export const Dup20Filled = 2;', fileName: 'dup-22-filled.tsx' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had "made up" patterns within test which doesn't exist, aligned now with real contents of /assets
| * @returns {Promise<void>} | ||
| * @throws Will throw an error if potential grouping issues are detected. | ||
| */ | ||
| async function assertCompoundStyleVariantIssues(destPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added build last line of defense to double check generated outputs from /assets, will fail build if there are issues
|
|
||
| const deprecatedMsg = `/** @deprecated use from @fluentui/${type}/color import. This was generated for backward compatibility and will be removed in next major release */`; | ||
| const generatedFiles = [] | ||
| const deprecatedMsg = getDeprecatedMsg(type, 'color'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRY applied
| writeSvg('ic_fluent_dup_20.svg', '<svg width="20" d="M0"></svg>'); | ||
| // this variant without underscore will camelCase to the same export name (Dup20) | ||
| writeSvg('ic_fluent_dup20.svg', '<svg width="20" d="M1"></svg>'); | ||
| writeSvg('ic_fluent_dup_20_regular.svg', '<svg width="20" d="M0"></svg>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updates test to reflect reality of /assets
5221bdb to
c27b3a8
Compare
c27b3a8 to
5a26eef
Compare
Before:
TextColor<size><variant>exports have been incorrectly groupped with Text undertext.tsxmoduleAfter:
TextColor<size><variants>exports groupped undertext-color.tsxText<size><variant>exports groupped withintext.tsxmodule@deprecationannotationThis fix enables build transforms via swc or babel without additional workarounds as initially documented https://github.com/microsoft/fluentui-system-icons/blob/main/packages/react-icons/README.md#using-api-via-build-transform