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 background color control for table block #10611

Merged
merged 16 commits into from Jan 18, 2019

Conversation

@talldan
Copy link
Contributor

talldan commented Oct 15, 2018

Description

Adds background and text color controls to the table block.

Works with both Regular and Stripes style variations of the table block.

How has this been tested?

  • Manual testing

Screenshots

screen shot 2018-12-28 at 5 45 31 pm

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@talldan talldan added the Blocks label Oct 15, 2018

@talldan talldan self-assigned this Oct 15, 2018

@talldan talldan requested review from jasmussen and jorgefilipecosta Oct 15, 2018

},
] }
>
<ContrastChecker

This comment was marked as outdated.

@talldan

talldan Oct 15, 2018

Author Contributor

Contrast checker doesn't work so well with the stripes variation, since on some rows the contrast might be fine, but on others the contract could be problematic. One way to solve could be to pass through multiple color groups in an array? Maybe something like an additionalColors prop. Alternatively, use two contrast checkers? That could get confusing if both are present at the same time.


const applyFallbackStyles = withFallbackStyles( ( node, props ) => {
const { textColor, backgroundColor } = props.attributes;
const styles = node ? getComputedStyle( node.querySelector( 'tr' ) ) : undefined;

This comment has been minimized.

@talldan

talldan Oct 15, 2018

Author Contributor

Currently only grabs fallback colors for the first row.

@karmatosed karmatosed self-requested a review Oct 15, 2018

@karmatosed
Copy link
Member

karmatosed left a comment

Let's scale this back a little to just having the background color and not a text color also. We should only have this for the stripes not for every cell right now. I have some ideas how we can do this and will comment in issue but let's also make sure this gets a 'design feedback' label as this goes in for Wednesday because a UI change.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Oct 15, 2018

We need to remove text color options and focus just on background ones that can't become inaccessible, just for stripes. As part of this, we should remove the color picker.

Color palette wise let's have it use a fixed palette. We have a few options:

  1. Use a light palette based on a set of colors:
    2018-10-15 at 12 39

  2. Desaturate and lighten the existing theme palette:

2018-10-15 at 12 47

Ideally, we would go for 2 but with time stop of Wednesday, let's make sure we get something in.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Oct 15, 2018

What if the stripes were a lighter or darker variant of the selected background color? Would that idea work?

@postphotos

This comment has been minimized.

Copy link
Contributor

postphotos commented Oct 15, 2018

What if the stripes were a lighter or darker variant of the selected background color? Would that idea work?

That could work as well!

@talldan talldan force-pushed the add/table-block-background-text-color branch from 7c024ea to b58fdcc Oct 16, 2018

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 16, 2018

I've refactored this quite a bit now from yesterday:

  • Removed text color controls.
  • Removed the ability to choose custom background colors.
  • Dynamically filtered out background colors that don't contrast with the default text color. (potentially though, this might exclude all colors for some themes? We could hide the color control for those themes.)
  • Use CSS only for styling background colors.

I think this makes the implementation a lot cleaner, and hopefully mergeable, but would love @jorgefilipecosta's feedback on the changes to color utils, ColorPalette and withColorContext.

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 16, 2018

Hi @karmatosed - My understanding is that the colors can be provided by the theme, so potentially the second option would be preferable so that the colors are still in keeping with the theme. Having said that, I'm not sure how we can lighten the colors dynamically if they're specified through css.

I think we'd have to only set the colors using inline styles (instead of classes) and lighten the colors in JavaScript. Potentially that could work, but might need input from a dev with more experience of themes / the color system to validate that approach.

At the moment I've used a different approach - I've filtered the color palette down to only accessible colors, which means the red and black colors are no longer available:
screen shot 2018-10-16 at 1 25 27 pm

I know this is not a particularly great set of colors to choose from. Three or four of them are ok.

Option 1 - I think I could make that work, but I worry about it being too prescriptive and maybe hard to undo in the future.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Hi @talldan I think if we manage to merge #10457 before this PR we may be able to simplify the changes here as it looks like the other PR is addressing a need from this PR.
I really like the feature being added here 👍
In the last version when I try to execute this branch I'm getting an error:

TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
    at edit.js:47
const settings = select( 'core/editor' ).getEditorSettings();
const colors = settings.colors;
const disableCustomColors = settings.disableCustomColors;
const disableCustomColors = settings.disableCustomColors || ownProps.disableCustomColors;

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Oct 16, 2018

Member

In PR #10457 I'm adding the possibility to overwrite color definitions for a specific component. If we manage the land it before it this PR it may be very useful and simplify the changes here.

Show resolved Hide resolved packages/components/src/color-palette/index.js Outdated

@talldan talldan changed the title Add background and text color controls for table block Add background color control for table block Oct 17, 2018

@talldan talldan force-pushed the add/table-block-background-text-color branch 3 times, most recently from 82d5ca7 to c6c4cc9 Oct 18, 2018

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 18, 2018

Hi @karmatosed - I had a go at lightening and desaturating the theme colors, but it ended up being very hit and miss, and was hard to find a happy medium that works for a decent selection. The current version of this branch goes with your first recommendation - there's a static selection of colors:
screen shot 2018-10-18 at 4 05 57 pm

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Hi @talldan this PR is testing well for me, nice work 👍
I think it would be perfect if we could avoid the use of inline styles I created a PR and left a comment with a possible path to get us there.

Show resolved Hide resolved packages/block-library/src/table/edit.js
@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Jan 16, 2019

Removing the Needs Design Feedback label as it looks like @karmatosed has given her feedback here.

@noisysocks
Copy link
Member

noisysocks left a comment

LGTM, minor comments aside 👍

I was confused by the createCustomColorsHOC API at first but the JSDoc code examples helped me eventually grok it. I think that using render props here might be simpler than a HOC but then again I think that about most of our HOCs, so... :shipit:😀


export default compose( [
withCustomBackgroundColors( 'backgroundColor' ),
] )( TableEdit );

This comment has been minimized.

@noisysocks

noisysocks Jan 17, 2019

Member

We don't need to use compose() here since it's just one HOC.

export default withCustomBackgroundColors( 'backgroundColor' )( TableEdit );

This comment has been minimized.

@talldan

talldan Jan 18, 2019

Author Contributor

Very true, addressed in 38a74ad

const classes = classnames( {
'has-fixed-layout': hasFixedLayout,
'has-background': !! backgroundClass,
[ backgroundClass ]: backgroundClass,
} );

This comment has been minimized.

@noisysocks

noisysocks Jan 17, 2019

Member

The object given to classnames should only map strings to booleans. I think for this use case we ought to pass backgroundClass as an argument.

const classes = classnames( backgroundClass, {
	'has-fixed-layout': hasFixedLayout,
	'has-background': !! backgroundClass,
} );

This comment has been minimized.

@talldan

talldan Jan 18, 2019

Author Contributor

Good spot! Addressed in 8fddd40

talldan and others added some commits Sep 13, 2018

Simplify implementation of colors for table block.
- Remove text color control
- Disable custom colors
- Apply class names to table element instead of tr element
- Remove JS striping
Revert "Filter colors in table background color control to only those…
… that contrast with the default text color"

This reverts commit 8a39fbc.
Add createCustomColorsHOC HOC builder that creates withColors HOC for…
… usages with custom colors array.

Refactor withColorHOC. Ensure withSelect is not used when not needed, and ensure custom color HOC has the right display name

Add tests for new createCustomColorsHOC
Remove contrast checker from table block background color selector - …
…table block has custom background colors designed to meet contrast requirements

@talldan talldan force-pushed the add/table-block-background-text-color branch from d64ba6b to 8fddd40 Jan 18, 2019

Requested changes have been addressed

@talldan talldan force-pushed the add/table-block-background-text-color branch from 673ef10 to 1384c41 Jan 18, 2019

@talldan talldan merged commit 3111e48 into master Jan 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the add/table-block-background-text-color branch Jan 18, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 18, 2019

👍 Nice

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