-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add icon element class names for animation #7729
Add icon element class names for animation #7729
Conversation
Deploy preview for carbon-elements ready! Built with commit de479b1 |
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit de479b1 https://deploy-preview-7729--carbon-components-react.netlify.app |
@lee-chase thanks so much for putting this together! It's awesome to see the generation logic in there. I'm a little hesitant about is adding this to the base packages, are there any other strategies we could use for enabling these kinds of transforms for adding classes to the paths? One thing that came to mind is a dedicated package that uses the icon data from |
@joshblack Another option would be to provide a wrapper to the Icon component, or perhaps just an alternate that walks the children and creates a component with classes added on the fly. The performance hit would be tiny on small SVGs. |
Perhaps we can get together to discuss how #7751 / #7346 are progressed. Are there others you would suggest joining such a conversation @joshblack ? |
@lee-chase I think moving forward @johnbister will be great on the design side. On the technical side, I think it'd be great to have a dedicated package for this kind of stuff that uses the icon data from I think from that experiment we can get a feel for how widespread this use-case is and how folks will want to leverage it before folding it into Let me know what you think! |
Will try and sort this at some point Josh, but struggling for time. |
Hey @lee-chase! 👋 Going to close this out since I think the group is going for a smaller approach first 👀 Let me know if this is not the case! |
DCO Assistant Lite bot: Thanks for your submission! We ask that you sign our Developer Certificate of Origin before we can accept your contribution. You can sign the DCO by adding a comment below using this text: I have read the DCO document and I hereby sign the DCO. Lee Chase seems not to be a GitHub user. You need a GitHub account to be able to sign the DCO. If you have already a GitHub account, please add the email address used for this commit to your account. |
Contributes to #7346
Currently, SVG Icon elements do not have classes to allow targetted CSS animation.
This PR adds the ability to utilise a className as part of the SVG metadata. It also adds a property to the Icon component to optionally add an automatically generated class based on the element type and index in the metadata.
NOTE 1: A slightly more robust auto generated class could be created, based on a selection of attribute values in the element.
NOTE 2: We could go a little further here and split
Path
elements on a 'z' and/or capitalized 'M' to make the animations more versatile. The same strategy used for differentiating auto generated classes could be used here.Changelog
Changed
M packages/icon-build-helpers/src/builders/react/builder.js
M packages/icon-build-helpers/src/builders/react/components/Icon.js
Testing / Reviewing
Tried it out in a test app with and without prop 'addElementClasses' also with and without a className attribute set in metadata.json.