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

Remove memoise withColors; Simplify API. #6782

Merged
merged 4 commits into from Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 1 addition & 8 deletions core-blocks/button/edit.js
Expand Up @@ -141,11 +141,4 @@ class ButtonEdit extends Component {
}
}

export default withColors( ( getColor, setColor, { attributes } ) => {
return {
backgroundColor: getColor( attributes.backgroundColor, attributes.customBackgroundColor, 'background-color' ),
setBackgroundColor: setColor( 'backgroundColor', 'customBackgroundColor' ),
textColor: getColor( attributes.textColor, attributes.customTextColor, 'color' ),
setTextColor: setColor( 'textColor', 'customTextColor' ),
};
} )( ButtonEdit );
export default withColors( 'backgroundColor', { textColor: 'color' } )( ButtonEdit );
13 changes: 3 additions & 10 deletions core-blocks/paragraph/index.js
Expand Up @@ -454,17 +454,10 @@ export const settings = {
}
},

edit: compose(
withColors( ( getColor, setColor, { attributes } ) => {
return {
backgroundColor: getColor( attributes.backgroundColor, attributes.customBackgroundColor, 'background-color' ),
setBackgroundColor: setColor( 'backgroundColor', 'customBackgroundColor' ),
textColor: getColor( attributes.textColor, attributes.customTextColor, 'color' ),
setTextColor: setColor( 'textColor', 'customTextColor' ),
};
} ),
edit: compose( [
withColors( 'backgroundColor', { textColor: 'color' } ),
FallbackStyles,
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: I don't think this should be capitalized as though it were a component, since it's not. Confused me thinking we had incorrect usage. Something with a with prefix would be good to reflect it being a higher-order component (admitting there is a bit of confusion on higher-order-components vs. higher-order-component-creators like withFallbackStyles).

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems withFallbackStyles is a better name I will rename it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #7355 so we don't forget (plus it's a good first bug).

)( ParagraphBlock ),
] )( ParagraphBlock ),

save( { attributes } ) {
const {
Expand Down
91 changes: 91 additions & 0 deletions editor/components/colors/with-colors-deprecated.js
@@ -0,0 +1,91 @@
/**
* External dependencies
*/
import memoize from 'memize';
import { get } from 'lodash';

/**
* WordPress dependencies
*/
import { createHigherOrderComponent, Component, compose } from '@wordpress/element';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { getColorValue, getColorClass, setColorValue } from './utils';

const DEFAULT_COLORS = [];

/**
* Higher-order component, which handles color logic for class generation
* color value, retrieval and color attribute setting.
*
* @param {Function} mapGetSetColorToProps Function that receives getColor, setColor, and props,
* and returns additional props to pass to the component.
*
* @return {Function} Higher-order component.
*/
export default ( mapGetSetColorToProps ) => createHigherOrderComponent(
compose( [
withSelect(
( select ) => {
const settings = select( 'core/editor' ).getEditorSettings();
return {
colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
};
} ),
( WrappedComponent ) => {
return class extends Component {
constructor() {
super( ...arguments );
/**
* Even though we don't expect setAttributes or colors to change memoizing it is essential.
* If setAttributes or colors are not memoized, each time memoizedGetColor/memoizedSetColor are called:
* a new function reference is returned (even if arguments have not changed).
* This would make our memoized chain useless.
*/
this.memoizedGetColor = memoize( this.memoizedGetColor, { maxSize: 1 } );
this.memoizedSetColor = memoize( this.memoizedSetColor, { maxSize: 1 } );
}

memoizedGetColor( colors ) {
return memoize(
( colorName, customColorValue, colorContext ) => {
return {
name: colorName,
class: getColorClass( colorContext, colorName ),
value: getColorValue( colors, colorName, customColorValue ),
};
}
);
}

memoizedSetColor( setAttributes, colors ) {
return memoize(
( colorNameAttribute, customColorAttribute ) => {
return setColorValue( colors, colorNameAttribute, customColorAttribute, setAttributes );
}
);
}

render() {
return (
<WrappedComponent
{ ...{
...this.props,
colors: undefined,
...mapGetSetColorToProps(
this.memoizedGetColor( this.props.colors ),
this.memoizedSetColor( this.props.setAttributes, this.props.colors ),
this.props
),
} }
/>
);
}
};
},
] ),
'withColors'
);
164 changes: 104 additions & 60 deletions editor/components/colors/with-colors.js
@@ -1,91 +1,135 @@
/**
* External dependencies
*/
import memoize from 'memize';
import { get } from 'lodash';
import { find, get, isFunction, isString, kebabCase, reduce, upperFirst } from 'lodash';

/**
* WordPress dependencies
*/
import { createHigherOrderComponent, Component, compose } from '@wordpress/element';
import { withSelect } from '@wordpress/data';
import { deprecated } from '@wordpress/utils';

/**
* Internal dependencies
*/
import { getColorValue, getColorClass, setColorValue } from './utils';
import { getColorValue, getColorClass } from './utils';
import withColorsDeprecated from './with-colors-deprecated';

const DEFAULT_COLORS = [];

/**
* Higher-order component, which handles color logic for class generation
* color value, retrieval and color attribute setting.
*
* @param {Function} mapGetSetColorToProps Function that receives getColor, setColor, and props,
* and returns additional props to pass to the component.
* @param {...(object|string)} args The arguments can be strings or objects. If the argument is an object,
* it should contain the color attribute name as key and the color context as value.
Copy link
Member

Choose a reason for hiding this comment

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

Can we define somewhere what we mean by "color context" in practice? Each time I look at this, I forget what it's for. I see it's primarily used as part of the generated class name modifier e.g. has-green-color ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I extended the comment to describe the color context.

* If the argument is a string the value should be the color attribute name,
* the color context is computed by applying a kebab case transform to the value.
Copy link
Member

Choose a reason for hiding this comment

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

Haha, I've never heard of kebab case 😄 🍖

* Color context represents the context/place where the color is going to be used.
* The class name of the color is generated using 'has' followed by the color name
* and ending with the color context all in kebab case e.g: has-green-background-color.
*
*
* @return {Function} Higher-order component.
*/
export default ( mapGetSetColorToProps ) => createHigherOrderComponent(
compose( [
withSelect(
( select ) => {
export default ( ...args ) => {
if ( isFunction( args[ 0 ] ) ) {
deprecated( 'Using withColors( mapGetSetColorToProps ) ', {
version: '3.3',
alternative: 'withColors( colorAttributeName, { secondColorAttributeName: \'color-context\' }, ... )',
} );
return withColorsDeprecated( args[ 0 ] );
}

const colorMap = reduce( args, ( colorObject, arg ) => {
return {
...colorObject,
...( isString( arg ) ? { [ arg ]: kebabCase( arg ) } : arg ),
};
}, {} );

return createHigherOrderComponent(
compose( [
withSelect( ( select ) => {
const settings = select( 'core/editor' ).getEditorSettings();
return {
colors: get( settings, [ 'colors' ], DEFAULT_COLORS ),
};
} ),
( WrappedComponent ) => {
return class extends Component {
constructor() {
super( ...arguments );
/**
* Even though, we don't expect setAttributes or colors to change memoizing it is essential.
* If setAttributes or colors are not memoized, each time memoizedGetColor/memoizedSetColor are called:
* a new function reference is returned (even if arguments have not changed).
* This would make our memoized chain useless.
*/
this.memoizedGetColor = memoize( this.memoizedGetColor, { maxSize: 1 } );
this.memoizedSetColor = memoize( this.memoizedSetColor, { maxSize: 1 } );
}
( WrappedComponent ) => {
return class extends Component {
constructor( props ) {
super( props );

this.setters = this.createSetters();

memoizedGetColor( colors ) {
return memoize(
( colorName, customColorValue, colorContext ) => {
return {
name: colorName,
class: getColorClass( colorContext, colorName ),
value: getColorValue( colors, colorName, customColorValue ),
};
}
);
}
this.state = {};
}

memoizedSetColor( setAttributes, colors ) {
return memoize(
( colorNameAttribute, customColorAttribute ) => {
return setColorValue( colors, colorNameAttribute, customColorAttribute, setAttributes );
}
);
}
createSetters() {
return reduce( colorMap, ( settersAccumulator, colorContext, colorAttributeName ) => {
const upperFirstColorAttributeName = upperFirst( colorAttributeName );
const customColorAttributeName = `custom${ upperFirstColorAttributeName }`;
settersAccumulator[ `set${ upperFirstColorAttributeName }` ] =
this.createSetColor( colorAttributeName, customColorAttributeName );
return settersAccumulator;
}, {} );
}

render() {
return (
<WrappedComponent
{ ...{
...this.props,
colors: undefined,
...mapGetSetColorToProps(
this.memoizedGetColor( this.props.colors ),
this.memoizedSetColor( this.props.setAttributes, this.props.colors ),
this.props
),
} }
/>
);
}
};
},
] ),
'withColors'
);
createSetColor( colorAttributeName, customColorAttributeName ) {
return ( colorValue ) => {
const colorObject = find( this.props.colors, { color: colorValue } );
this.props.setAttributes( {
[ colorAttributeName ]: colorObject && colorObject.name ? colorObject.name : undefined,
[ customColorAttributeName ]: colorObject && colorObject.name ? undefined : colorValue,
} );
};
}

static getDerivedStateFromProps( { attributes, colors }, previousState ) {
return reduce( colorMap, ( newState, colorContext, colorAttributeName ) => {
const colorName = attributes[ colorAttributeName ];
const colorValue = getColorValue(
colors,
colorName,
attributes[ `custom${ upperFirst( colorAttributeName ) }` ]
);
const previousColorObject = previousState[ colorAttributeName ];
const previousColorValue = get( previousColorObject, [ 'value' ] );
/**
* The "and previousColorObject" condition checks that a previous color object was already computed.
* At the start previousColorObject and colorValue are both equal to undefined
* bus as previousColorObject does not exist we should compute the object.
*/
if ( previousColorValue === colorValue && previousColorObject ) {
newState[ colorAttributeName ] = previousColorObject;
} else {
newState[ colorAttributeName ] = {
name: colorName,
class: getColorClass( colorContext, colorName ),
value: colorValue,
};
}
return newState;
}, {} );
}

render() {
return (
<WrappedComponent
{ ...{
...this.props,
colors: undefined,
...this.state,
...this.setters,
} }
/>
);
}
};
},
] ),
'withColors'
);
};