Skip to content

Send props to Dashicon.#14631

Merged
gziolo merged 10 commits intomasterfrom
rnmobile/add-block-icon-color-v2
Mar 27, 2019
Merged

Send props to Dashicon.#14631
gziolo merged 10 commits intomasterfrom
rnmobile/add-block-icon-color-v2

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Mar 26, 2019 •
edited
Loading

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#628

Dashicon was modified to allow set colors and style on Mobile.
With these modifications, we can now do this:

<ToolbarButton
	label={ __( 'Add block' ) }
	icon={ ( <Dashicon icon="plus-alt" style={ styles.addBlockButton } color={ styles.addBlockButton.color } /> ) }
	onClick={ onInsertClick }
	iconProps={ { style: styles.addBlockButton, color: styles.addBlockButton.color } }
/>

And let ToolbarButton (or IconButton) use a Dashicon 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.

@etoledom etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Mar 26, 2019
@etoledom etoledom self-assigned this Mar 26, 2019
@etoledom etoledom requested review from Tug and aduth March 26, 2019 10:49
This was referenced Mar 26, 2019
Merged
Closed
@Tug
Copy link
Copy Markdown
Contributor

Tug commented Mar 26, 2019

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?

Tug
Tug approved these changes Mar 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Tug Tug left a 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 🚢

gziolo
gziolo approved these changes Mar 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gziolo gziolo left a 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 🙇

@gziolo gziolo added the [Feature] UI Components Impacts or related to the UI component system label Mar 27, 2019
@gziolo gziolo added this to the 5.4 (Gutenberg) milestone Mar 27, 2019
@gziolo gziolo merged commit b79710b into master Mar 27, 2019
@youknowriad youknowriad deleted the rnmobile/add-block-icon-color-v2 branch March 27, 2019 09:57
@aduth
Copy link
Copy Markdown
Member

aduth commented Mar 27, 2019

It's not clear to me from the follow-up work whether it's still intended to remove ariaPressed as a prop of Dashicon.

This was referenced Apr 1, 2019
Merged
Merged
@koke koke mentioned this pull request Oct 3, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tug Tug Tug approved these changes

@gziolo gziolo gziolo approved these changes

@aduth aduth Awaiting requested review from aduth

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@noisysocks noisysocks Awaiting requested review from noisysocks

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@etoledom etoledom

Labels

[Feature] UI Components Impacts or related to the UI component system Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Milestone

5.4 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

4 participants

@etoledom @Tug @aduth @gziolo