From 9589ffe603c31f09a96064bd377dbf2ea9c0e881 Mon Sep 17 00:00:00 2001 From: Marin Atanasov Date: Mon, 17 Oct 2022 14:11:44 +0300 Subject: [PATCH 1/5] Lodash: Refactor away from _.omit() in addSaveProps() --- packages/block-editor/src/hooks/style.js | 125 +++++++++++- packages/block-editor/src/hooks/test/style.js | 180 +++++++++++++++++- 2 files changed, 301 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/hooks/style.js b/packages/block-editor/src/hooks/style.js index f7dfff746e3f0..d2e9ad5a8fc5a 100644 --- a/packages/block-editor/src/hooks/style.js +++ b/packages/block-editor/src/hooks/style.js @@ -1,7 +1,6 @@ /** * External dependencies */ -import { omit } from 'lodash'; import classnames from 'classnames'; /** @@ -134,6 +133,126 @@ const skipSerializationPathsSave = { */ const renamedFeatures = { gradients: 'gradient' }; +/** + * A utility function used to remove one or more paths from a style object. + * Works in a way similar to Lodash's `omit()`. See unit tests and examples below. + * + * It supports a single string path: + * + * ``` + * omitStyle( { color: 'red' }, 'color' ); // {} + * ``` + * + * or an array of paths: + * + * ``` + * omitStyle( { color: 'red', background: '#fff' }, [ 'color', 'background' ] ); // {} + * ``` + * + * It also allows you to specify paths at multiple levels in a string. + * + * ``` + * omitStyle( { typography: { textDecoration: 'underline' } }, 'typography.textDecoration' ); // {} + * ``` + * + * You can remove multiple paths at the same time: + * + * ``` + * omitStyle( + * { + * typography: { + * textDecoration: 'underline', + * textTransform: 'uppercase', + * } + * }, + * [ + * 'typography.textDecoration', + * 'typography.textTransform', + * ] + * ); + * // {} + * ``` + * + * You can also specify nested paths as arrays: + * + * ``` + * omitStyle( + * { + * typography: { + * textDecoration: 'underline', + * textTransform: 'uppercase', + * } + * }, + * [ + * [ 'typography', 'textDecoration' ], + * [ 'typography', 'textTransform' ], + * ] + * ); + * // {} + * ``` + * + * With regards to nesting of styles, infinite depth is supported: + * + * ``` + * omitStyle( + * { + * border: { + * radius: { + * topLeft: '10px', + * topRight: '0.5rem', + * } + * } + * }, + * [ + * [ 'border', 'radius', 'topRight' ], + * ] + * ); + * // { border: { radius: { topLeft: '10px' } } } + * ``` + * + * The third argument, `preserveReference`, defines how to treat the input style object. + * It is mostly necessary to properly handle mutation when recursively handling the style object. + * Defaulting to `false`, this will always create a new object, avoiding to mutate `style`. + * However, when recursing, we change that value to `true` in order to work with a single copy + * of the original style object. + * + * @see https://lodash.com/docs/4.17.15#omit + * + * @param {Object} style Styles object. + * @param {Array|string} paths Paths to remove. + * @param {boolean} preserveReference True to mutate the `style` object, false otherwise. + * @return {Object} Styles object with the specified paths removed. + */ +export function omitStyle( style, paths, preserveReference = false ) { + if ( ! style ) { + return style; + } + + let newStyle = style; + if ( ! preserveReference ) { + newStyle = JSON.parse( JSON.stringify( style ) ); + } + + if ( ! Array.isArray( paths ) ) { + paths = [ paths ]; + } + + paths.forEach( ( path ) => { + if ( ! Array.isArray( path ) ) { + path = path.split( '.' ); + } + + if ( path.length > 1 ) { + const [ firstSubpath, ...restPath ] = path; + omitStyle( newStyle[ firstSubpath ], [ restPath ], true ); + } else { + delete newStyle[ path ]; + } + } ); + + return newStyle; +} + /** * Override props assigned to save component to inject the CSS variables definition. * @@ -159,13 +278,13 @@ export function addSaveProps( const skipSerialization = getBlockSupport( blockType, indicator ); if ( skipSerialization === true ) { - style = omit( style, path ); + style = omitStyle( style, path ); } if ( Array.isArray( skipSerialization ) ) { skipSerialization.forEach( ( featureName ) => { const feature = renamedFeatures[ featureName ] || featureName; - style = omit( style, [ [ ...path, feature ] ] ); + style = omitStyle( style, [ [ ...path, feature ] ] ); } ); } } ); diff --git a/packages/block-editor/src/hooks/test/style.js b/packages/block-editor/src/hooks/test/style.js index 40cc8f49c59f5..d82b25d9b781c 100644 --- a/packages/block-editor/src/hooks/test/style.js +++ b/packages/block-editor/src/hooks/test/style.js @@ -6,7 +6,7 @@ import { applyFilters } from '@wordpress/hooks'; /** * Internal dependencies */ -import { getInlineStyles } from '../style'; +import { getInlineStyles, omitStyle } from '../style'; describe( 'getInlineStyles', () => { it( 'should return an empty object when called with undefined', () => { @@ -202,3 +202,181 @@ describe( 'addSaveProps', () => { } ); } ); } ); + +describe( 'omitStyle', () => { + it( 'should remove a single path', () => { + const style = { color: '#d92828', padding: '10px' }; + const path = 'color'; + const expected = { padding: '10px' }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should remove multiple paths', () => { + const style = { color: '#d92828', padding: '10px', background: 'red' }; + const path = [ 'color', 'background' ]; + const expected = { padding: '10px' }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should remove nested paths when specified as a string', () => { + const style = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + textTransform: 'uppercase', + }, + }; + const path = 'typography.textTransform'; + const expected = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + }, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should remove nested paths when specified as an array', () => { + const style = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + textTransform: 'uppercase', + }, + }; + const path = [ [ 'typography', 'textTransform' ] ]; + const expected = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + }, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should remove multiple nested paths', () => { + const style = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + textTransform: 'uppercase', + }, + }; + const path = [ + [ 'typography', 'textTransform' ], + 'typography.textDecoration', + ]; + const expected = { + color: { + text: '#d92828', + }, + typography: {}, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should paths with different nesting', () => { + const style = { + color: { + text: '#d92828', + }, + typography: { + textDecoration: 'underline', + textTransform: 'uppercase', + }, + }; + const path = [ + 'color', + [ 'typography', 'textTransform' ], + 'typography.textDecoration', + ]; + const expected = { + typography: {}, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should support beyond 2 levels of nesting when passed as a single string', () => { + const style = { + border: { + radius: { + topLeft: '10px', + topRight: '0.5rem', + }, + }, + }; + const path = 'border.radius.topRight'; + const expected = { + border: { + radius: { + topLeft: '10px', + }, + }, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should support beyond 2 levels of nesting when passed as array of strings', () => { + const style = { + border: { + radius: { + topLeft: '10px', + topRight: '0.5rem', + }, + }, + }; + const path = [ 'border.radius.topRight' ]; + const expected = { + border: { + radius: { + topLeft: '10px', + }, + }, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should support beyond 2 levels of nesting when passed as array of arrays', () => { + const style = { + border: { + radius: { + topLeft: '10px', + topRight: '0.5rem', + }, + }, + }; + const path = [ [ 'border', 'radius', 'topRight' ] ]; + const expected = { + border: { + radius: { + topLeft: '10px', + }, + }, + }; + + expect( omitStyle( style, path ) ).toEqual( expected ); + } ); + + it( 'should ignore a nullish style object', () => { + expect( omitStyle( undefined, 'color' ) ).toEqual( undefined ); + expect( omitStyle( null, 'color' ) ).toEqual( null ); + } ); +} ); From ae0d16e03ac77ca165f9a4ec539a3258f15a0d88 Mon Sep 17 00:00:00 2001 From: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Date: Tue, 18 Oct 2022 13:31:01 +0300 Subject: [PATCH 2/5] Fix whitespacing in JSdoc as per review suggestions Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --- packages/block-editor/src/hooks/style.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/block-editor/src/hooks/style.js b/packages/block-editor/src/hooks/style.js index d2e9ad5a8fc5a..929a955ae5bf9 100644 --- a/packages/block-editor/src/hooks/style.js +++ b/packages/block-editor/src/hooks/style.js @@ -165,7 +165,7 @@ const renamedFeatures = { gradients: 'gradient' }; * textTransform: 'uppercase', * } * }, - * [ + * [ * 'typography.textDecoration', * 'typography.textTransform', * ] @@ -183,7 +183,7 @@ const renamedFeatures = { gradients: 'gradient' }; * textTransform: 'uppercase', * } * }, - * [ + * [ * [ 'typography', 'textDecoration' ], * [ 'typography', 'textTransform' ], * ] @@ -203,7 +203,7 @@ const renamedFeatures = { gradients: 'gradient' }; * } * } * }, - * [ + * [ * [ 'border', 'radius', 'topRight' ], * ] * ); From 87c6c55ce731091a0d3718bcd0cd6b7a17763cb0 Mon Sep 17 00:00:00 2001 From: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Date: Tue, 18 Oct 2022 13:31:49 +0300 Subject: [PATCH 3/5] Fix typo in test name Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --- packages/block-editor/src/hooks/test/style.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/block-editor/src/hooks/test/style.js b/packages/block-editor/src/hooks/test/style.js index d82b25d9b781c..146ad0bc4c6b0 100644 --- a/packages/block-editor/src/hooks/test/style.js +++ b/packages/block-editor/src/hooks/test/style.js @@ -290,7 +290,7 @@ describe( 'omitStyle', () => { expect( omitStyle( style, path ) ).toEqual( expected ); } ); - it( 'should paths with different nesting', () => { + it( 'should remove paths with different nesting', () => { const style = { color: { text: '#d92828', From a691f88127024509158fbec89bda4f4f2ff7f010 Mon Sep 17 00:00:00 2001 From: Marin Atanasov Date: Tue, 18 Oct 2022 13:38:07 +0300 Subject: [PATCH 4/5] Add test for no property match for given path --- packages/block-editor/src/hooks/test/style.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/block-editor/src/hooks/test/style.js b/packages/block-editor/src/hooks/test/style.js index 146ad0bc4c6b0..4eed7663434df 100644 --- a/packages/block-editor/src/hooks/test/style.js +++ b/packages/block-editor/src/hooks/test/style.js @@ -379,4 +379,24 @@ describe( 'omitStyle', () => { expect( omitStyle( undefined, 'color' ) ).toEqual( undefined ); expect( omitStyle( null, 'color' ) ).toEqual( null ); } ); + + it( 'should ignore a missing object property', () => { + const style1 = { typography: {} }; + expect( omitStyle( style1, 'color' ) ).toEqual( style1 ); + + const style2 = { color: { text: '#d92828' } }; + expect( omitStyle( style2, 'color.something' ) ).toEqual( style2 ); + + const style3 = { + border: { + radius: { + topLeft: '10px', + topRight: '0.5rem', + }, + }, + }; + expect( + omitStyle( style3, [ [ 'border', 'radius', 'bottomLeft' ] ] ) + ).toEqual( style3 ); + } ); } ); From 4e3c1f9c98cf76dff3dff7b80aa3b6a19d8f4b38 Mon Sep 17 00:00:00 2001 From: Marin Atanasov Date: Wed, 19 Oct 2022 11:16:40 +0300 Subject: [PATCH 5/5] Fix array access, add length check and additional test --- packages/block-editor/src/hooks/style.js | 4 ++-- packages/block-editor/src/hooks/test/style.js | 7 +++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/hooks/style.js b/packages/block-editor/src/hooks/style.js index 929a955ae5bf9..d701342d4cb33 100644 --- a/packages/block-editor/src/hooks/style.js +++ b/packages/block-editor/src/hooks/style.js @@ -245,8 +245,8 @@ export function omitStyle( style, paths, preserveReference = false ) { if ( path.length > 1 ) { const [ firstSubpath, ...restPath ] = path; omitStyle( newStyle[ firstSubpath ], [ restPath ], true ); - } else { - delete newStyle[ path ]; + } else if ( path.length === 1 ) { + delete newStyle[ path[ 0 ] ]; } } ); diff --git a/packages/block-editor/src/hooks/test/style.js b/packages/block-editor/src/hooks/test/style.js index 4eed7663434df..26c2522e79f1b 100644 --- a/packages/block-editor/src/hooks/test/style.js +++ b/packages/block-editor/src/hooks/test/style.js @@ -399,4 +399,11 @@ describe( 'omitStyle', () => { omitStyle( style3, [ [ 'border', 'radius', 'bottomLeft' ] ] ) ).toEqual( style3 ); } ); + + it( 'should ignore an empty array path', () => { + const style = { typography: {}, '': 'test' }; + + expect( omitStyle( style, [] ) ).toEqual( style ); + expect( omitStyle( style, [ [] ] ) ).toEqual( style ); + } ); } );