Skip to content

"Add Block" icon color#628

Merged
etoledom merged 19 commits intodevelopfrom
issue/546-add-block-icon-color
Mar 27, 2019
Merged

"Add Block" icon color#628
etoledom merged 19 commits intodevelopfrom
issue/546-add-block-icon-color

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Feb 20, 2019 •
edited
Loading

fixes: #546

This PR implements the changes made in WordPress/gutenberg#14631 to change the default color of Dashicons (in this case on ToolbarButton).

img_1268

To test:

  • Build the app.
  • Check that the add-block button is filled with $blue-wordpress color.

@etoledom etoledom added the [Type] Enhancement Improves a current area of the editor label Feb 20, 2019
@etoledom etoledom added this to the Beta milestone Feb 20, 2019
@etoledom etoledom self-assigned this Feb 20, 2019
@etoledom etoledom requested a review from Tug February 20, 2019 16:38
@iamthomasbishop
Copy link
Copy Markdown
Contributor

iamthomasbishop commented Feb 20, 2019

Is this using a Dashicon? If so, can we either use a custom SVG or Material icon here? This icon feels a little off.

@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 20, 2019

Is this using a Dashicon? If so, can we either use a custom SVG or Material icon here? This icon feels a little off.

Yes it is Dashicon. The ToolbarButton component is made on top of Dashicon component.
We can have a custom SVG, we did it before for the hide-keyboard icon. But it's kinda hacky 🤔

If the previous icon looks better compared to this one, maybe we can wait a bit for this change?

Tug
Tug reviewed Feb 20, 2019
View reviewed changes
src/block-management/block-toolbar.js Outdated
icon="insert"
icon="plus-alt"
onClick={ onInsertClick }
extraProps={ styles.addBlockButton }
Copy link
Copy Markdown
Contributor

@Tug Tug Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't be setting the icon's color attribute as it will be prone to error if we add css rules to this addBlockButton class. Also it's best practice to group styling in style or className.
So:

extraProps={ { style: styles.addBlockButton } }

@etoledom etoledom mentioned this pull request Feb 20, 2019
Closed
@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 20, 2019

Thanks @Tug and @iamthomasbishop for the review!

I implemented the suggested changes and tweaked a bit the look of the icon as Thomas suggested, adding a border to it. Now it looks 😎

file

@iamthomasbishop
Copy link
Copy Markdown
Contributor

iamthomasbishop commented Feb 20, 2019

That looks so much better. Thank you! <3

Tug
Tug approved these changes Feb 21, 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.

LGTM 👍

@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 21, 2019

Updated with the changes requested for a better clarity on the intention.
Thank you @Tug !

@hypest hypest modified the milestones: Beta, v1.0 Feb 22, 2019
@hypest hypest modified the milestones: v1.0, Future Feb 22, 2019
@etoledom etoledom modified the milestones: Future, v1.2 Mar 26, 2019
@etoledom etoledom mentioned this pull request Mar 26, 2019
Merged
@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Mar 26, 2019

I updated the implementation of this, with a new gutenberg side PR: WordPress/gutenberg#14631

Would you mind @Tug taking another look?
Thank you!

@etoledom etoledom requested a review from Tug March 26, 2019 10:50
Tug
Tug reviewed Mar 26, 2019
View reviewed changes
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.

Works well, good job here 🎉

@etoledom etoledom changed the title Issue/546 add block icon color "Add Block" icon color Mar 26, 2019
@etoledom etoledom merged commit 48e5251 into develop Mar 27, 2019
@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Mar 27, 2019

Thank you!

@etoledom etoledom deleted the issue/546-add-block-icon-color branch March 27, 2019 08:47
@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

1 more reviewer

@Tug Tug Tug approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@etoledom etoledom

Labels

[Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

v1.2

Development

Successfully merging this pull request may close these issues.

Update Inserter toggle icon style

4 participants

@etoledom @iamthomasbishop @Tug @hypest