-
Notifications
You must be signed in to change notification settings - Fork 2
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 theme
prop to Icons
#1155
Add theme
prop to Icons
#1155
Conversation
🦋 Changeset detectedLatest commit: 741a15b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Tip Once this PR is ready to go, add the This saves us a lot of money by not running the tests before we need them. |
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.
Am I correct in thinking that only monochrome icons can be themed?
Yes, I think that makes sense. What do you think? |
Yes, I think that enabling polychromatic icons to be styled is going to be a much larger endeavour that is best tackled only if consumers demand it. |
@@ -327,19 +331,6 @@ MediumIconsBrandTheme.parameters = { | |||
default: 'brandBackground.primary', | |||
}, | |||
}; | |||
MediumIconsBrandTheme.decorators = [ |
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.
Glad to see these style overrides removed and replaced with a supported method so we're not encouraging people to do the wrong thing 🙈
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.
Nice work 🚀
094d0a0
into
release/source-react-components-themeing
What are you changing?
Why?