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

ColorPalette: white palette color merging with white background #5598

Merged

Conversation

lukekowalski
Copy link
Contributor

ColorPalette block allows to provide custom colors but in case we have white (#fff) color among them it may be confusing for the users as it will merge with the background.

This PR solves it by adding a thin border around the white colour so it stands out (border color matches the border from '.components-panel__color-area' class)

How Has This Been Tested?

Insert ColorPalette component in some part of the editor i.e: existing 'paragraph' block for test purposes and verify that white colour stands out.
Sample:

<PanelColor title={ __( 'Text Color' ) } colorValue={ textColor } initialOpen={ false }>
	<ColorPalette
		colors={ [ '#fff', '#000', '#ccc' ] }
		value={ textColor }
		onChange={ ( colorValue ) => setAttributes( { textColor: colorValue } ) }
	/>
</PanelColor>

Screenshots (jpeg or gifs if applicable):

Before fix:
image

After fix:
image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

Fixing white color from ColorPalette block merging with the background
@bordoni
Copy link
Member

bordoni commented Mar 14, 2018

Why not add transparent border for all with 20% opacity from black? it removes the conditional but and applies to colors that are very close to white too. like #fefefe

Adding black border with 20% opacity to all colors to cover multiple scenarios (all colors that are close to white + multiple ways of declaring white in the ColorPalette)
@lukekowalski
Copy link
Contributor Author

That's a very good point. I guess I've seen border:none there and I just focused on not altering the initial designs too much. Your approach is much better though as not only doesn't require conditional but also it covers multiple scenarios and ways to declare white color in the ColorPalette (ie: #fff, #ffffff, white ). Applied this small change.

Preview available below:
image

@paulwilde
Copy link
Contributor

paulwilde commented Mar 15, 2018

Using a pseudo class to add an inset box-shadow would allow for the border to match the same colour as each option, as otherwise the grey kinda looks bad against darker colours.

.blocks-color-palette__item:after {
    content: '';
    position: absolute;
    top: 0;
    left: 0;
    bottom: 0;
    right: 0;
    border-radius: 50%;
    box-shadow: inset 0 0 0 1px rgba(0, 0, 0, .2);
}

screen shot 2018-03-15 at 09 13 28

@lukekowalski
Copy link
Contributor Author

That's super clever. Thanks for digging into it!

inset box-shadow allows for the border to match the same colour as each option
@lukekowalski lukekowalski changed the title ColorPalette: white palette color meging with white background ColorPalette: white palette color merging with white background Mar 15, 2018
@aduth aduth added the Needs Design Feedback Needs general design feedback. label Mar 20, 2018
@karmatosed
Copy link
Member

@paulwilde assuming this works on all colors this is the preferred way for me: #5598 (comment)

Thanks for digging into this everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants