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

"Add Block" icon color #628

Merged
merged 19 commits into from
Mar 27, 2019
Merged

"Add Block" icon color #628

merged 19 commits into from
Mar 27, 2019

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Feb 20, 2019

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
Contributor

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
Contributor Author

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?

onClick={ onInsertClick }
extraProps={ styles.addBlockButton }
Copy link
Contributor

@Tug Tug Feb 20, 2019

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
Copy link
Contributor Author

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
Contributor

That looks so much better. Thank you! <3

Copy link
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
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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
Contributor Author

Thank you!

@etoledom etoledom deleted the issue/546-add-block-icon-color branch March 27, 2019 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Inserter toggle icon style
4 participants