-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Send props to Dashicon. #14631
Send props to Dashicon. #14631
Conversation
I feel we've came back to the original logic now, I don't feel the proposed changes either improve or decrease the quality or logic of the code. Am I missing something? |
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.
Ok those were changes suggested in #13977
LGTM 🚢
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 is what we discussed in #13977. Let's 🚢 Thanks for wrangling it 🙇
It's not clear to me from the follow-up work whether it's still intended to remove |
This PR is a continuation of #13977, and was simplified thanks to this suggestion: #13977 (comment) from @aduth .
gutenberg-mobile
side PR: wordpress-mobile/gutenberg-mobile#628Dashicon was modified to allow set colors and style on Mobile.
With these modifications, we can now do this:
And let
ToolbarButton
(orIconButton
) use aDashicon
component setup on any way we need it to.I will modify https://github.com/WordPress/dashicons/tree/master/sources/react after this PR gets merged.