-
Notifications
You must be signed in to change notification settings - Fork 371
feat(CC-icons): added support scripts #11794
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
base: main
Are you sure you want to change the base?
Conversation
3a3df97
to
3b8fe71
Compare
Preview: https://patternfly-react-pr-11794.surge.sh A11y report: https://patternfly-react-pr-11794-a11y.surge.sh |
'Font size - 2XL': 'font-size---2xl', | ||
'Font size - 3XL': 'font-size---3xl', | ||
'Font size - 4XL': 'font-size---4xl' | ||
}) |
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.
Add the children
prop as the actual icon displayed is passed as a child of the component.
}) | |
}), | |
children: figma.children('*') |
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.
Talking about changing this from children to instance...
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.
Add to follow-up issue
// Read the existing icons data | ||
let config; | ||
try { | ||
const configPath = path.resolve(process.cwd(), 'codeConnect/config.json'); |
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.
This file doesn't exist so the code always falls to the catch
, should this file be part of this PR?
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.
No extra file needed - we'll open follow-up issue to review & potentially remove this unused code.
const importStatement = `import React from "react"; | ||
import { | ||
${componentNames.join(',\n ')} | ||
} from "@patternfly/react-icons/dist/esm/icons"; |
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.
FYI running the script, this imports all icons from this one import path as seen below:
WindowsIcon,
WrenchIcon,
ZoneIcon
} from "@patternfly/react-icons/dist/esm/icons";
The existing icons.figma.tsx file in this PR has separate imports as so:
import WindowsIcon from './@patternfly/react-icons/dist/esm/icons/windows-icon';
import WrenchIcon from './@patternfly/react-icons/dist/esm/icons/wrench-icon';
import ZoneIcon from './@patternfly/react-icons/dist/esm/icons/zone-icon';
Mostly small changes that should be quick, but we should also make the FYI - for this PR the API call to fetch icons is erroring out, I can confirm this was working previously but for now we can use the hard-coded icons data that you've included and address this in a follow-up enhancement. |
@@ -0,0 +1,31 @@ | |||
import figma from '@figma/code-connect'; |
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.
Not this line but the file name - missed earlier that the file can be renamed from "IconWrapper" to just "Icon" to match the component name.
closes: #11620
Installation instructions
Then copy the token and save somewhere safe.
This PR provides all necessary files to:
iconsData.json
In following PRs: