From dbbff80f7944b2e41d02d5a01ddc9591c664eb5d Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Tue, 19 Oct 2021 17:51:28 +1100 Subject: [PATCH 1/3] Border Radius Control: Add fallback px unit and add utils tests --- .../components/border-radius-control/index.js | 3 +- .../border-radius-control/test/utils.js | 216 ++++++++++++++++++ .../components/border-radius-control/utils.js | 29 ++- 3 files changed, 237 insertions(+), 11 deletions(-) create mode 100644 packages/block-editor/src/components/border-radius-control/test/utils.js diff --git a/packages/block-editor/src/components/border-radius-control/index.js b/packages/block-editor/src/components/border-radius-control/index.js index ed154caf4e3bfc..ae501b60d68400 100644 --- a/packages/block-editor/src/components/border-radius-control/index.js +++ b/packages/block-editor/src/components/border-radius-control/index.js @@ -53,7 +53,8 @@ export default function BorderRadiusControl( { onChange, values } ) { const units = useCustomUnits( { availableUnits: useSetting( 'spacing.units' ) || [ 'px', 'em', 'rem' ], } ); - const unit = getAllUnit( values ) || 'px'; + + const unit = getAllUnit( values ); const unitConfig = units && units.find( ( item ) => item.value === unit ); const step = unitConfig?.step || 1; diff --git a/packages/block-editor/src/components/border-radius-control/test/utils.js b/packages/block-editor/src/components/border-radius-control/test/utils.js new file mode 100644 index 00000000000000..3d148901c8eadd --- /dev/null +++ b/packages/block-editor/src/components/border-radius-control/test/utils.js @@ -0,0 +1,216 @@ +/** + * Internal dependencies + */ +import { + getAllUnit, + getAllValue, + hasMixedValues, + hasDefinedValues, + mode, +} from '../utils'; + +describe( 'getAllUnit', () => { + describe( 'when provided string based values', () => { + it( 'should return valid unit when passed a valid unit', () => { + expect( getAllUnit( '32em' ) ).toBe( 'em' ); + } ); + + it( 'should fall back to px when passed an invalid unit', () => { + expect( getAllUnit( '32apples' ) ).toBe( 'px' ); + } ); + + it( 'should fall back to px when passed a value without a unit', () => { + expect( getAllUnit( '32' ) ).toBe( 'px' ); + } ); + } ); + + describe( 'when provided object based values', () => { + it( 'should return the most common value', () => { + const values = { + bottomLeft: '2em', + bottomRight: '2em', + topLeft: '0', + topRight: '2px', + }; + expect( getAllUnit( values ) ).toBe( 'em' ); + } ); + + it( 'should return the real value when the most common value is undefined', () => { + const values = { + bottomLeft: '0', + bottomRight: '0', + topLeft: '0', + topRight: '2em', + }; + expect( getAllUnit( values ) ).toBe( 'em' ); + } ); + + it( 'should return the most common value there are no undefined values', () => { + const values = { + bottomLeft: '1em', + bottomRight: '1em', + topLeft: '2px', + topRight: '2em', + }; + expect( getAllUnit( values ) ).toBe( 'em' ); + } ); + + it( 'should fall back to px when all values are undefined or equivalent', () => { + const values = { + bottomLeft: '0', + bottomRight: undefined, + topLeft: undefined, + topRight: '0', + }; + expect( getAllUnit( values ) ).toBe( 'px' ); + } ); + } ); + + describe( 'when provided invalid values', () => { + it( 'should return px when passed an array', () => { + expect( getAllUnit( [] ) ).toBe( 'px' ); + } ); + it( 'should return px when passed a boolean', () => { + expect( getAllUnit( false ) ).toBe( 'px' ); + } ); + it( 'should return px when passed undefined', () => { + expect( getAllUnit( false ) ).toBe( 'px' ); + } ); + } ); +} ); + +describe( 'getAllValue', () => { + describe( 'when provided string based values', () => { + it( 'should return valid value + unit when passed a valid unit', () => { + expect( getAllValue( '32em' ) ).toBe( '32em' ); + } ); + + it( 'should return string as-is without passing it', () => { + expect( getAllValue( '32apples' ) ).toBe( '32apples' ); + } ); + } ); + + describe( 'when provided object based values', () => { + it( 'should return null if values are mixed', () => { + const values = { + bottomLeft: '2em', + bottomRight: '2em', + topLeft: '0', + topRight: '2px', + }; + expect( getAllValue( values ) ).toBe( null ); + } ); + + it( 'should return the common value + unit when all values are the same', () => { + const values = { + bottomLeft: '1em', + bottomRight: '1em', + topLeft: '1em', + topRight: '1em', + }; + expect( getAllValue( values ) ).toBe( '1em' ); + } ); + + it( 'should return the common value + most common unit when same values but different units', () => { + const values = { + bottomLeft: '1em', + bottomRight: '1em', + topLeft: '1px', + topRight: '1rem', + }; + expect( getAllValue( values ) ).toBe( '1em' ); + } ); + + it( 'should fall back to null when values are undefined', () => { + const values = { + bottomLeft: undefined, + bottomRight: undefined, + topLeft: undefined, + topRight: undefined, + }; + expect( getAllValue( values ) ).toBe( null ); + } ); + } ); + + describe( 'when provided invalid values', () => { + it( 'should return px when passed an array', () => { + expect( getAllValue( [] ) ).toBe( null ); + } ); + it( 'should return px when passed a boolean', () => { + expect( getAllValue( false ) ).toBe( null ); + } ); + it( 'should return px when passed undefined', () => { + expect( getAllValue( false ) ).toBe( null ); + } ); + } ); +} ); + +describe( 'hasMixedValues', () => { + it( 'should return false when passed a string value', () => { + expect( hasMixedValues( '2px' ) ).toBe( false ); + } ); + + it( 'should return true when passed mixed values', () => { + const values = { + bottomLeft: '1em', + bottomRight: '1px', + topLeft: '2px', + topRight: '2em', + }; + expect( hasMixedValues( values ) ).toBe( true ); + } ); + + it( 'should return false when passed a common value', () => { + const values = { + bottomLeft: '1em', + bottomRight: '1em', + topLeft: '1em', + topRight: '1em', + }; + expect( hasMixedValues( values ) ).toBe( false ); + } ); +} ); + +describe( 'hasDefinedValues', () => { + it( 'should return false when passed a falsy value', () => { + expect( hasDefinedValues( undefined ) ).toBe( false ); + expect( hasDefinedValues( null ) ).toBe( false ); + expect( hasDefinedValues( '' ) ).toBe( false ); + } ); + + it( 'should return true when passed a non empty string value', () => { + expect( hasDefinedValues( '1px' ) ).toBe( true ); + } ); + + it( 'should return false when passed an object with empty values', () => { + const values = { + bottomLeft: undefined, + bottomRight: undefined, + topLeft: undefined, + topRight: undefined, + }; + expect( hasDefinedValues( values ) ).toBe( false ); + } ); + + it( 'should return true when passed an object with at least one real value', () => { + const values = { + bottomLeft: undefined, + bottomRight: '1px', + topLeft: undefined, + topRight: undefined, + }; + expect( hasDefinedValues( values ) ).toBe( true ); + } ); +} ); + +describe( 'mode', () => { + it( 'should return the most common value', () => { + const values = [ 'a', 'z', 'z', 'b', undefined ]; + expect( mode( values ) ).toBe( 'z' ); + } ); + + it( 'should return the most common real value', () => { + const values = [ undefined, 'a', undefined, undefined, undefined ]; + expect( mode( values ) ).toBe( 'a' ); + } ); +} ); diff --git a/packages/block-editor/src/components/border-radius-control/utils.js b/packages/block-editor/src/components/border-radius-control/utils.js index a5bf6e176c92db..cded43f6139777 100644 --- a/packages/block-editor/src/components/border-radius-control/utils.js +++ b/packages/block-editor/src/components/border-radius-control/utils.js @@ -4,27 +4,36 @@ import { __experimentalParseUnit as parseUnit } from '@wordpress/components'; /** - * Gets the item with the highest occurrence within an array - * https://stackoverflow.com/a/20762713 + * Gets the (non-undefined) item with the highest occurrence within an array + * Based in part on: https://stackoverflow.com/a/20762713 * - * @param {Array} arr Array of items to check. - * @return {any} The item with the most occurrences. + * Undefined values are always sorted to the end by `sort`, so this function + * returns the first element, to always prioritize real values over undefined + * values. See: + * + * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#description + * + * @param {Array} inputArray Array of items to check. + * @return {any} The item with the most occurrences. */ -function mode( arr ) { +export function mode( inputArray ) { + const arr = [ ...inputArray ]; return arr .sort( ( a, b ) => - arr.filter( ( v ) => v === a ).length - - arr.filter( ( v ) => v === b ).length + inputArray.filter( ( v ) => v === b ).length - + inputArray.filter( ( v ) => v === a ).length ) - .pop(); + .shift(); } /** * Returns the most common CSS unit in the radius values. * + * Fallback to `px` as a default unit. + * * @param {Object|string} values Radius values. - * @return {string} Most common CSS unit in values. + * @return {string} Most common CSS unit in values. Default: `px`. */ export function getAllUnit( values = {} ) { if ( typeof values === 'string' ) { @@ -37,7 +46,7 @@ export function getAllUnit( values = {} ) { return unit; } ); - return mode( allUnits ); + return mode( allUnits ) || 'px'; } /** From daaca27723e69c786eb5565f005d1eff94fbc75c Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Wed, 20 Oct 2021 10:44:57 +1100 Subject: [PATCH 2/3] Tweak comments --- .../src/components/border-radius-control/utils.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/components/border-radius-control/utils.js b/packages/block-editor/src/components/border-radius-control/utils.js index cded43f6139777..0f628d6dce068b 100644 --- a/packages/block-editor/src/components/border-radius-control/utils.js +++ b/packages/block-editor/src/components/border-radius-control/utils.js @@ -9,9 +9,9 @@ import { __experimentalParseUnit as parseUnit } from '@wordpress/components'; * * Undefined values are always sorted to the end by `sort`, so this function * returns the first element, to always prioritize real values over undefined - * values. See: + * values. * - * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#description + * See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#description * * @param {Array} inputArray Array of items to check. * @return {any} The item with the most occurrences. @@ -29,8 +29,7 @@ export function mode( inputArray ) { /** * Returns the most common CSS unit in the radius values. - * - * Fallback to `px` as a default unit. + * Falls back to `px` as a default unit. * * @param {Object|string} values Radius values. * @return {string} Most common CSS unit in values. Default: `px`. From 3646d98d04945e765dbf48748a66b0d1468d6552 Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Wed, 20 Oct 2021 15:24:40 +1100 Subject: [PATCH 3/3] Fix typo (change "passing" to "parsing") Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --- .../src/components/border-radius-control/test/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/components/border-radius-control/test/utils.js b/packages/block-editor/src/components/border-radius-control/test/utils.js index 3d148901c8eadd..50d932322a02f3 100644 --- a/packages/block-editor/src/components/border-radius-control/test/utils.js +++ b/packages/block-editor/src/components/border-radius-control/test/utils.js @@ -85,7 +85,7 @@ describe( 'getAllValue', () => { expect( getAllValue( '32em' ) ).toBe( '32em' ); } ); - it( 'should return string as-is without passing it', () => { + it( 'should return string as-is without parsing it', () => { expect( getAllValue( '32apples' ) ).toBe( '32apples' ); } ); } );