-
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
feat: allow users to set styles on icons in Vue #4786
feat: allow users to set styles on icons in Vue #4786
Conversation
Deploy preview for the-carbon-components ready! Built with commit d606533 https://deploy-preview-4786--the-carbon-components.netlify.com |
Deploy preview for carbon-elements ready! Built with commit d606533 |
Deploy preview for carbon-components-react ready! Built with commit d606533 https://deploy-preview-4786--carbon-components-react.netlify.com |
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 believe the merge conflict is due to your previous PR, would you mind resolving that conflict?
Merged |
|
||
// convert incoming style attr to object form. | ||
const styleArray = style.split(';').filter(item => item.trim().length > 0); | ||
const styleObj = styleArray.reduce((acc, styleString) => { |
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.
Would you want to do this at build time versus compile time? In our base component for React we end up just building the object in scope:
This value used to derive from the value coming from icon-helpers (e.g. we parsed the string value) but now we just inline it to make things easier.
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.
Is this what you were suggesting @joshblack ?
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.
do the bit of refactoring @joshblack mentioned and this looks good to me 👍
Closes #4785
Allows user to specify styles dynamically in a Vue template, including overriding individual styles such as 'will-change' where set by default in getAttributes.
Changelog
M packages/icons-vue/tasks/tests/createIconComponent-test.js
M packages/icons-vue/tasks/createIconComponent.js