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 componentWillReceiveProps from ColorPicker #11772

Merged
merged 41 commits into from May 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f62cd92
Tests for onChange prop
oandregal Nov 13, 2018
f832d72
Tests for onChange prop not being called
oandregal Nov 13, 2018
b87fb10
Add is rendered tests
oandregal Nov 13, 2018
78f3171
Input values are lowercased on mount
oandregal Nov 13, 2018
b456119
Input values are lowercased on subsequent props updates
oandregal Nov 13, 2018
64d2636
Remove unnecessary onChange declaration
oandregal Nov 13, 2018
19d8639
Extract toLowerCase function
oandregal Nov 13, 2018
f081ee7
Remove componentWillReceiveProps lifecycle method
oandregal Nov 13, 2018
828a0fd
Create a separate path for handling input data
oandregal Nov 14, 2018
6a4f78f
Add a state prop to the data lifted up
oandregal Nov 14, 2018
d327ce3
Add draft values to state
oandregal Nov 14, 2018
7f84889
Make Input a fully controlled component
oandregal Nov 14, 2018
b13e35d
Flatten draft values
oandregal Nov 14, 2018
822c7f2
Consider handleChange event always a draft operation
oandregal Nov 14, 2018
0fc4339
Lift HEX validation up
oandregal Nov 14, 2018
fe4f830
Test ColorPicker: use uppercase HEX color value
oandregal Nov 14, 2018
e4c37e7
Test ColorPicker: uses React TestRenderer instead of ShallowRenderer
oandregal Nov 14, 2018
0066ffc
Test ColorPicker: draft changes onChange event
oandregal Nov 14, 2018
e999df4
Test ColorPicker: commit changes onBlur
oandregal Nov 14, 2018
657af92
Test ColorPicker: commit changes onKeyDown UP
oandregal Nov 14, 2018
b60b770
Test ColorPicker: commit changes onKeyDown DOWN
oandregal Nov 14, 2018
35ec3fc
Test ColorPicker: commit changes onKeyDown ENTER
oandregal Nov 14, 2018
f023388
Test Input: update tests
oandregal Nov 14, 2018
a428e4d
Fire commit or draft change depending on the value length
oandregal Nov 14, 2018
b04f979
Delete unused snapshots tests
oandregal Nov 14, 2018
8c31eb1
Test Input: update tests for different value lengths
oandregal Nov 14, 2018
a044a92
Make Saturation, Hue, and Alpha pure components
oandregal Nov 15, 2018
0a00c1d
Make Icon pure
oandregal Nov 15, 2018
ce76b67
Update snapshot
oandregal Nov 15, 2018
20b9e18
Allow introducing empty values
oandregal Jan 29, 2019
7d3ea74
Do not commit empty values
oandregal Jan 29, 2019
8ffdbd5
Reset draft values when views are toggled.
oandregal Jan 29, 2019
1264fc3
Consider Saturation and Hue when the source=rgb
oandregal Jan 29, 2019
97469bd
Rename handleChange to commitValues
oandregal Feb 20, 2019
8122124
Extract set and reset draft values
oandregal Feb 20, 2019
6d8e0d3
Use switch instead of if/else
oandregal Feb 20, 2019
4f575ab
Refactor source to be taken from view
oandregal Feb 20, 2019
05494bc
Refactor how the colors object is updated.
oandregal Feb 20, 2019
7986e09
Fix linter issue: dependencies comment block
oandregal Feb 20, 2019
8108977
Update tests to new API
oandregal Feb 20, 2019
4c202f7
Remove unnecessary check for Saturation and Hue
oandregal Mar 15, 2019
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
Expand Up @@ -7,7 +7,7 @@ exports[`ColorPalette Dropdown .renderContent should render dropdown content 1`]
<div
className="components-color-picker__saturation"
>
<WithInstanceId(Saturation)
<Pure(WithInstanceId(Saturation))
hsl={
Object {
"a": 1,
Expand Down Expand Up @@ -48,7 +48,7 @@ exports[`ColorPalette Dropdown .renderContent should render dropdown content 1`]
<div
className="components-color-picker__toggles"
>
<WithInstanceId(Hue)
<Pure(WithInstanceId(Hue))
hsl={
Object {
"a": 1,
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/color-picker/alpha.js
Expand Up @@ -36,6 +36,7 @@ import { noop } from 'lodash';
import { __ } from '@wordpress/i18n';
import { Component, createRef } from '@wordpress/element';
import { TAB } from '@wordpress/keycodes';
import { pure } from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -174,4 +175,4 @@ export class Alpha extends Component {
}
}

export default Alpha;
export default pure( Alpha );
7 changes: 5 additions & 2 deletions packages/components/src/color-picker/hue.js
Expand Up @@ -33,7 +33,7 @@ import { noop } from 'lodash';
/**
* WordPress dependencies
*/
import { withInstanceId } from '@wordpress/compose';
import { compose, pure, withInstanceId } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { Component, createRef } from '@wordpress/element';
import { TAB } from '@wordpress/keycodes';
Expand Down Expand Up @@ -171,4 +171,7 @@ export class Hue extends Component {
}
}

export default withInstanceId( Hue );
export default compose(
pure,
withInstanceId
)( Hue );
154 changes: 137 additions & 17 deletions packages/components/src/color-picker/index.js
Expand Up @@ -43,30 +43,150 @@ import Alpha from './alpha';
import Hue from './hue';
import Inputs from './inputs';
import Saturation from './saturation';
import { colorToState, simpleCheckForValidColor } from './utils';
import { colorToState, simpleCheckForValidColor, isValidHex } from './utils';

const toLowerCase = ( value ) => String( value ).toLowerCase();
const isValueEmpty = ( data ) => {
if ( data.source === 'hex' && ! data.hex ) {
return true;
} else if ( data.source === 'hsl' && ( ! data.h || ! data.s || ! data.l ) ) {
return true;
} else if ( data.source === 'rgb' && (
( ! data.r || ! data.g || ! data.b ) &&
( ! data.h || ! data.s || ! data.v || ! data.a ) &&
( ! data.h || ! data.s || ! data.l || ! data.a )
Copy link
Contributor

@ryelle ryelle Mar 5, 2019

Choose a reason for hiding this comment

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

These values above can be zero, so this is causing some trouble with using the keyboard to move through the saturation. If you focus into the saturation handle, you can use shift + left or shift + right (or just left/right) to move the handle, but you can't get it all the way to the edge.

colors-key

I'm pretty sure this is also the root of this issue, both seemed fixed if I change this to check for isFinite but of course that breaks the inputs :|

Edit (again): played with this more, you can use an isFinite check if you cast the value to an int in normalizeValue 👍

) ) {
return true;
}
return false;
};
const isValidColor = ( colors ) => colors.hex ?
isValidHex( colors.hex ) :
simpleCheckForValidColor( colors );

/**
* Function that creates the new color object
* from old data and the new value.
*
* @param {Object} oldColors The old color object.
* @param {string} oldColors.hex
* @param {Object} oldColors.rgb
* @param {number} oldColors.rgb.r
* @param {number} oldColors.rgb.g
* @param {number} oldColors.rgb.b
* @param {number} oldColors.rgb.a
* @param {Object} oldColors.hsl
* @param {number} oldColors.hsl.h
* @param {number} oldColors.hsl.s
* @param {number} oldColors.hsl.l
* @param {number} oldColors.hsl.a
* @param {string} oldColors.draftHex Same format as oldColors.hex
* @param {Object} oldColors.draftRgb Same format as oldColors.rgb
* @param {Object} oldColors.draftHsl Same format as oldColors.hsl
* @param {Object} data Data containing the new value to update.
* @param {Object} data.source One of `hex`, `rgb`, `hsl`.
* @param {string\number} data.value Value to update.
* @param {string} data.valueKey Depends on `data.source` values:
* - when source = `rgb`, valuKey can be `r`, `g`, `b`, or `a`.
* - when source = `hsl`, valuKey can be `h`, `s`, `l`, or `a`.
* @return {Object} A new color object for a specific source. For example:
* { source: 'rgb', r: 1, g: 2, b:3, a:0 }
*/
const dataToColors = ( oldColors, { source, valueKey, value } ) => {
if ( source === 'hex' ) {
return {
source,
[ source ]: value,
};
}
return {
source,
...{ ...oldColors[ source ], ...{ [ valueKey ]: value } },
};
};

export default class ColorPicker extends Component {
constructor( { color = '0071a1' } ) {
super( ...arguments );
this.state = colorToState( color );
this.handleChange = this.handleChange.bind( this );
const colors = colorToState( color );
this.state = {
...colors,
draftHex: toLowerCase( colors.hex ),
draftRgb: colors.rgb,
draftHsl: colors.hsl,
};
this.commitValues = this.commitValues.bind( this );
this.setDraftValues = this.setDraftValues.bind( this );
this.resetDraftValues = this.resetDraftValues.bind( this );
this.handleInputChange = this.handleInputChange.bind( this );
}

handleChange( data ) {
commitValues( data ) {
const { oldHue, onChangeComplete = noop } = this.props;
const isValidColor = simpleCheckForValidColor( data );
if ( isValidColor ) {

if ( isValidColor( data ) ) {
const colors = colorToState( data, data.h || oldHue );
this.setState(
colors,
debounce( partial( onChangeComplete, colors ), 100 )
this.setState( {
...colors,
draftHex: toLowerCase( colors.hex ),
draftHsl: colors.hsl,
draftRgb: colors.rgb,
},
debounce( partial( onChangeComplete, colors ), 100 )
);
}
}

resetDraftValues() {
this.setState( {
draftHex: this.state.hex,
draftHsl: this.state.hsl,
draftRgb: this.state.rgb,
} );
}

setDraftValues( data ) {
switch ( data.source ) {
case 'hex':
this.setState( { draftHex: toLowerCase( data.hex ) } );
break;
case 'rgb':
this.setState( { draftRgb: data } );
break;
case 'hsl':
this.setState( { draftHsl: data } );
break;
}
}

handleInputChange( data ) {
switch ( data.state ) {
case 'reset':
this.resetDraftValues();
break;
case 'commit':
const colors = dataToColors( this.state, data );
if ( ! isValueEmpty( colors ) ) {
this.commitValues( colors );
}
break;
case 'draft':
this.setDraftValues( dataToColors( this.state, data ) );
break;
}
}

render() {
const { className, disableAlpha } = this.props;
const { color, hex, hsl, hsv, rgb } = this.state;
const {
color,
hsl,
hsv,
rgb,
draftHex,
draftHsl,
draftRgb,
} = this.state;
const classes = classnames( className, {
'components-color-picker': true,
'is-alpha-disabled': disableAlpha,
Expand All @@ -79,7 +199,7 @@ export default class ColorPicker extends Component {
<Saturation
hsl={ hsl }
hsv={ hsv }
onChange={ this.handleChange }
onChange={ this.commitValues }
/>
</div>

Expand All @@ -95,22 +215,22 @@ export default class ColorPicker extends Component {
</div>

<div className="components-color-picker__toggles">
<Hue hsl={ hsl } onChange={ this.handleChange } />
<Hue hsl={ hsl } onChange={ this.commitValues } />
{ disableAlpha ? null : (
<Alpha
rgb={ rgb }
hsl={ hsl }
onChange={ this.handleChange }
onChange={ this.commitValues }
/>
) }
</div>
</div>

<Inputs
rgb={ rgb }
hsl={ hsl }
hex={ hex }
onChange={ this.handleChange }
rgb={ draftRgb }
hsl={ draftHsl }
hex={ draftHex }
onChange={ this.handleInputChange }
disableAlpha={ disableAlpha }
/>
</div>
Expand Down