-
Notifications
You must be signed in to change notification settings - Fork 23
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(codemod): add codemod to upgrade Icon to Future Icon #5080
feat(codemod): add codemod to upgrade Icon to Future Icon #5080
Conversation
…erial-symbols--codemod
packages/components/codemods/upgradeIconV1/getNewIconPropsFromOldIconName.ts
Show resolved
Hide resolved
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.
Awesome work
export const TestComponent = () => ( | ||
<> | ||
<Brand variant="enso" style={{ width: "20px" }} role="presentation" /> | ||
<Brand variant="enso" style={{ width: "20px" }} role="img" aria-label="Add something" /> |
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.
Shouldn't this remove role="img"
and change aria-label
to alt
?
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.
Oops! Yep you're right 😬 Will update 🙏🏻
["VisibleIcon", { name: "visibility" }], | ||
["WritingIcon", undefined], | ||
["ZoomInIcon", { name: "zoom_in" }], | ||
["ZoomOutIcon", { name: "zoom_out" }], |
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.
Wow this is a bonkers long list, kudos on getting the map 👏
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.
Copy/paste is our best friend <3
value: string | ||
): ts.JsxAttribute => createProp(name, ts.factory.createStringLiteral(value)) | ||
|
||
export const createStyleProp = ( |
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.
Seem potentially handy for the future. Could we add a util description here just to tether it back to its main purpose? I'm thinkin' something like 'A util that creates a style prop with the given attributes to be consumed in a transformer, ie: adding style={{width: "500px"}}
".
*/ | ||
export const getKaioTagNamesByRegex = ( | ||
node: ts.Node, | ||
importSpecifierTarget: string |
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.
Should we mentioned regex in this arg? I know we call out that this performs on regex in the code comments but took me a second to realised this would also accept regex
/* Consumer helpers */ | ||
export const setImportToRemove = ( |
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.
I kind of understand these in the abstract by looking at how they're used in the upgradeIconV1 but can we add a little more context in the code comments, potentially with an example?
…icon-material-symbols--codemod
2c7b013
into
KZN-2759/release--icon-material-symbols
* feat(Icon): add future Icon (#4973) * feat: add illustrations entrypoint * chore(storybook): link to Material Symbols CDN * feat(future/Icon): add prop to mirror in RTL * docs(future/Icon): add api specification * docs(future/Icon): update usage guidelines * feat(future/Icon): add props for accessibility * feat(future/Icon): add string suggestions for name * feat(future/Icon): handle RTL icons that are not a direct mirror * docs(future/Icon): add list of default icon set * docs(DosAndDonts): update styles so items are the same height * chore: update root test command to not watch * docs(DosAndDonts): update styles for single col to span full width * feat(Notification): update icons to use future Icon * chore(tsconfig): resolve json in storybook * chore(DosAndDonts): update to use new icon * chore(ResourceLinks): update to use new icon * fix(future/Tag): fix cautionary icon colour * refactor: update Icon usage in some components * chore: fix lint:fix script * fix(Popover): fix icon color * feat(codemod): add codemod to upgrade Icon to Future Icon (#5080) * refactor: rename getTagName to getKaioTagName and abstract traversing * feat(codemod): add util getKaioTagNamesByRegex * refactor(transformSource): make tag name generic * feat(updateJsxElementWithNewProps): add arg to update tag name * feat(codemod): add codemod for upgrading icons * feat(upgradeIconV1): transform CaMonogramIcon to Brand * feat(updateKaioImport): add util to update imports * feat(upgradeIconV1): transform SpinnerIcon to LoadingSpinner
Why
KZN-2677
What
README:
CaMonogramIcon:
enso
inheritSize
- remove propinheritSize
- add style to set size to 20pxSpinnerIcon:
aria-label
withaccessibilityLabel
or fallback of"Loading"
role
viewBox
LiveIcon:
sensors
Icon:
name
based on previous component nameisFilled
if required based on previous component namerole="presentation"
becomesisPresentational
role="img"
to be removed (should have anaria-label
)aria-label
becomesalt
classNameOverride
becomesclassName
inheritSize
as is (expecting a TS error) and leave a comment above itaria-hidden
toisPresentational
color
- inline styledata-testid
as isfontSize
- doesn't do anythingheight
+width
- convert to inline styleid
as isonClick
as isviewBox