Skip to content
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

fix: dynamic classes only now functional #4780

Merged
merged 3 commits into from
Nov 28, 2019

Conversation

lee-chase
Copy link
Member

Closes #4779

Add the ability to use dynamic classes only in @carbon/icons-vue

Changelog

m packages/icons-vue/tasks/tests/createIconComponent-test.js
m packages/icons-vue/tasks/createIconComponent.js

Testing / Reviewing

Without this I am unable to progress the use of carbon prefix for classes in some @carbon/vue components.

@lee-chase lee-chase requested a review from a team as a code owner November 26, 2019 16:02
@ghost ghost requested review from asudoh and emyarod November 26, 2019 16:02
@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for the-carbon-components ready!

Built with commit d2f8d43

https://deploy-preview-4780--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for carbon-elements ready!

Built with commit d2f8d43

https://deploy-preview-4780--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for carbon-components-react ready!

Built with commit d2f8d43

https://deploy-preview-4780--carbon-components-react.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 - Thanks @lee-chase!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@@ -55,6 +55,7 @@ export default {
};
}
if (data.class) {
svgData.class = svgData.class || {}; // may be no static class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just wondering could the default value for svgData.class be an empty object as well? on L48:

const svgData = {
  attrs,
  on: listeners,
  class: {},
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it will do any harm, I was just trying to make the minimal impact change.

@lee-chase
Copy link
Member Author

Hey @asudoh @emyarod any chance this could make it into the next release? It is preventing me replacing 'bx' prefixes on classes with the carbon setting in @carbon/vue

@emyarod emyarod merged commit c170f74 into carbon-design-system:master Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic classes only added to icons when static classes used
3 participants