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 API to configure icon colors #7068

Merged
merged 1 commit into from Jun 4, 2018

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented May 31, 2018

Description

This PR allows the icon to configure 3 colors: backgroundColor, iconColor, shadowColor.
If iconColor and shadowColor are not passed they are automatically computed from the
backgroundColor.
This colors right now are being used in the inserter. Shadow color is only used in blocks that have children.

Some discussion points:
Should we use this colors in other places?
Should we try another way for blocks to configure this colors?

Closes: #6998

How has this been tested?

This change is very risky as we are changing how an icon is represented, try to do some smoke testing around Gutenberg in places where we show block icons and see things work as expected.
Register some test blocks that make sure colors work as expected. Test blocks available in https://gist.github.com/jorgefilipecosta/2dd281f9f5f078258f7c8d4ba4cc34cd ( can be pasted in developer console).

Screeshots:

screen shot 2018-05-31 at 20 39 02

screen shot 2018-05-31 at 20 38 53

*
* @type {Object}
*/
const ICON_COLORS = [ '#191e23', '#fefdfe', '#7e70af' ];

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

Minor: By convention, I've come to expect all module constants to be defined at the top of the file, immediately following dependencies. Should we consider that here?

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

Where do these colors come from? Are we duplicating from stylesheet variables?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

The colors are from the stylesheet variables $dark-gray-900 and $light-gray-100. Before we used a scss importer but with our divisions in packages I'm not certain if this is something we should pursue now.

* @property {string} category Category classification of block,
* impacting where block is shown in
* inserter results.
* @property {(Object|string|WPElement)} icon Slug of the Dashicon to be shown

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

Thinking at some point we might just want to @typedef a WPIcon, since it's used pretty broadly now.

http://usejsdoc.org/tags-typedef.html

Related: #5965 (comment) (cc @ajitbohra)

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Yes, here what we have is (Object|WPIcon) I think when we have generic Icon component in @wordpress/components we should create the type there so the type can be imported from this module to other modules.

This comment has been minimized.

@mtias

mtias Jun 1, 2018

Contributor

Note that the plan is to move away from UI icons for blocks and into custom svgs, so this will become more of a fallback.

icon = { src: icon };
}
if ( icon.backgroundColor && ( ! icon.iconColor || ! icon.shadowColor ) ) {

This comment has been minimized.

@aduth

aduth May 31, 2018

Member

Should we allow shadow color to be customizable?

Otherwise, we could simplify this to "background" and "foreground". icon.iconColor sounds a bit redundant.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

I followed this idea 👍

@mtias mtias added this to the 3.0 milestone Jun 1, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 1, 2018

This PR was updated in rebased taking into account the changes in #7074.

*/
export function normalizeIconObject( icon ) {
if ( ! icon ) {
icon = { src: 'block-default' };

This comment has been minimized.

@youknowriad

youknowriad Jun 4, 2018

Contributor

Can we just return?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 4, 2018

Member

Updated 👍

icon = { src: 'block-default' };
}
if ( isString( icon ) || icon instanceof Component ) {
icon = { src: icon };

This comment has been minimized.

@youknowriad

youknowriad Jun 4, 2018

Contributor

Same

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 4, 2018

Should we update the Block API docs?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 4, 2018

Not sure, what I'm doing wrong, the colors are not showing up for me. (using the gist)

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 4, 2018

Hi @youknowriad thank you for the review!

Should we update the Block API docs?

I expanded the docs to include this use case.

Not sure, what I'm doing wrong, the colors are not showing up for me. (using the gist)

I'm sorry I missed an update to the gist, with changes applied during the revisions. The Gist was updated and things should work now.

@youknowriad

LGTM 👍

Re Docs: Actually, I was referring to these docs https://wordpress.org/gutenberg/handbook/block-api/#icon-optional

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 4, 2018

Re Docs: Actually, I was referring to these docs wordpress.org/gutenberg/handbook/block-api/#icon-optional

I added an updated to this docs 👍

Hi @karmatosed could we have a check regarding the design and UX?

@mtias

This comment has been minimized.

Contributor

mtias commented Jun 4, 2018

Looks good to me, used it to create screenshots for the release post.

@karmatosed

I am approving as this does work. I will add a note though we need to do something about checking accessibility issues such as contrast. We can also easily now have a white background with white icon. That's not great and something maybe we should check to fix.

@jorgefilipecosta jorgefilipecosta merged commit aa5cc43 into master Jun 4, 2018

2 checks passed

codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +6.97% compared to b763387
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/api-configura-icon-colors branch Jun 4, 2018

jorgefilipecosta added a commit that referenced this pull request Jun 5, 2018

Fix svg block icons; Add icon validation and test cases (#7140)
The condition to check SVG icons was not correct. Now we use a more specific function that validates the icon.
Test cases were added.

Fixes #7139.
Regressed in #7068.
@maximebj

This comment has been minimized.

Contributor

maximebj commented Jun 12, 2018

I've made a PR for the documentation to add an example with foreground color and shadow color.

By the way, shadowColor doesn't seems to work for me as shown in the screenshot. Any idea why?

Also, the fallback with most readable for the foreground color is not always perfect : sometimes with a dark color, the svg color stays black.

@maximebj

This comment has been minimized.

Contributor

maximebj commented Jun 12, 2018

Oh sorry, I just figured out that shadowColor is only for parent block with a stack style.
And is automatically defined (make sense).

To be CSS consistent, how about use color instead of foreground ?

@aduth

This comment has been minimized.

Member

aduth commented Jun 25, 2018

To be CSS consistent, how about use color instead of foreground ?

Can you clarify what's meant by consistency here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment