From 5d986b585955bc8d540857000c618bae6c5956ec Mon Sep 17 00:00:00 2001 From: John Godley Date: Thu, 26 Jul 2018 16:17:33 +0100 Subject: [PATCH 1/2] Fix deletion of custom block classes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you add a custom class to a block and then remove that custom class it would flag the block as invalid. The reason is that the custom class became part of the blocks ‘default’ attributes, and it would then add it back and this then the expected HTML wouldn’t match the current HTML. This change ensures the block contains the default class names + whatever classes are currently in the HTML, ignoring any custom ones in the block itself. It also handles the situation where the block's default class doesn't exist in the HTML The unit tests have been beefed up to cope with several edge case situations --- .../editor/src/hooks/custom-class-name.js | 12 ++++-- .../src/hooks/test/custom-class-name.js | 40 +++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/editor/src/hooks/custom-class-name.js b/packages/editor/src/hooks/custom-class-name.js index 8cde23572ea7a1..c69143fda71936 100644 --- a/packages/editor/src/hooks/custom-class-name.js +++ b/packages/editor/src/hooks/custom-class-name.js @@ -136,15 +136,21 @@ export function addParsedDifference( blockAttributes, blockType, innerHTML ) { // To determine difference, serialize block given the known set of // attributes. If there are classes which are mismatched with the // incoming HTML of the block, add to filtered result. + // If existing classes have been removed then ensure they don't appear in the output const serialized = getSaveContent( blockType, blockAttributes ); + const blockClasses = blockAttributes.className ? blockAttributes.className.split( ' ' ) : []; const classes = getHTMLRootElementClasses( serialized ); const parsedClasses = getHTMLRootElementClasses( innerHTML ); const customClasses = difference( parsedClasses, classes ); - const filteredClassName = compact( [ - blockAttributes.className, + // The remaining classes consist of the block's current classes (with default classes removed if they don't already exist in the block) + // along with any custom classes, removing any class that doesn't appear in the innerHTML + const remaining = [ + ...classes.filter( ( item ) => blockClasses.indexOf( item ) !== -1 ), ...customClasses, - ] ).join( ' ' ); + ].filter( ( item ) => parsedClasses.indexOf( item ) !== -1 ); + + const filteredClassName = compact( remaining ).join( ' ' ); if ( filteredClassName ) { blockAttributes.className = filteredClassName; diff --git a/packages/editor/src/hooks/test/custom-class-name.js b/packages/editor/src/hooks/test/custom-class-name.js index 3414889aff7c5e..c7f1b3e1c1c5c5 100644 --- a/packages/editor/src/hooks/test/custom-class-name.js +++ b/packages/editor/src/hooks/test/custom-class-name.js @@ -110,5 +110,45 @@ describe( 'custom className', () => { expect( attributes.className ).toBeUndefined(); } ); + + it( 'should add a custom class name to an element without a class', () => { + const attributes = addParsedDifference( + {}, + blockSettings, + '
' + ); + + expect( attributes.className ).toBe( 'foo' ); + } ); + + it( 'should remove the custom class and retain default class', () => { + const attributes = addParsedDifference( + { className: 'default custom' }, + blockSettings, + '
' + ); + + expect( attributes.className ).toBe( 'default' ); + } ); + + it( 'should remove the custom class from an element originally without a class', () => { + const attributes = addParsedDifference( + { className: 'foo' }, + blockSettings, + '
' + ); + + expect( attributes.className ).toBeUndefined(); + } ); + + it( 'should remove the custom classes and retain default and other custom class', () => { + const attributes = addParsedDifference( + { className: 'default custom1 custom2 custom3' }, + blockSettings, + '
' + ); + + expect( attributes.className ).toBe( 'default custom1 custom3' ); + } ); } ); } ); From 1f2a7a19387d18a33097af2641c8edd3e30d7ce2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Fri, 3 Aug 2018 16:07:29 -0400 Subject: [PATCH 2/2] Editor: Reimplement custom class hook as difference on default --- .../editor/src/hooks/custom-class-name.js | 33 +++++++------------ .../src/hooks/test/custom-class-name.js | 16 ++++----- 2 files changed, 20 insertions(+), 29 deletions(-) diff --git a/packages/editor/src/hooks/custom-class-name.js b/packages/editor/src/hooks/custom-class-name.js index c69143fda71936..093863fbb7bd0b 100644 --- a/packages/editor/src/hooks/custom-class-name.js +++ b/packages/editor/src/hooks/custom-class-name.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { assign, difference, compact } from 'lodash'; +import { assign, difference, omit } from 'lodash'; import classnames from 'classnames'; /** @@ -134,26 +134,17 @@ export function getHTMLRootElementClasses( innerHTML ) { export function addParsedDifference( blockAttributes, blockType, innerHTML ) { if ( hasBlockSupport( blockType, 'customClassName', true ) ) { // To determine difference, serialize block given the known set of - // attributes. If there are classes which are mismatched with the - // incoming HTML of the block, add to filtered result. - // If existing classes have been removed then ensure they don't appear in the output - const serialized = getSaveContent( blockType, blockAttributes ); - const blockClasses = blockAttributes.className ? blockAttributes.className.split( ' ' ) : []; - const classes = getHTMLRootElementClasses( serialized ); - const parsedClasses = getHTMLRootElementClasses( innerHTML ); - const customClasses = difference( parsedClasses, classes ); - - // The remaining classes consist of the block's current classes (with default classes removed if they don't already exist in the block) - // along with any custom classes, removing any class that doesn't appear in the innerHTML - const remaining = [ - ...classes.filter( ( item ) => blockClasses.indexOf( item ) !== -1 ), - ...customClasses, - ].filter( ( item ) => parsedClasses.indexOf( item ) !== -1 ); - - const filteredClassName = compact( remaining ).join( ' ' ); - - if ( filteredClassName ) { - blockAttributes.className = filteredClassName; + // attributes, with the exception of `className`. This will determine + // the default set of classes. From there, any difference in innerHTML + // can be considered as custom classes. + const attributesSansClassName = omit( blockAttributes, [ 'className' ] ); + const serialized = getSaveContent( blockType, attributesSansClassName ); + const defaultClasses = getHTMLRootElementClasses( serialized ); + const actualClasses = getHTMLRootElementClasses( innerHTML ); + const customClasses = difference( actualClasses, defaultClasses ); + + if ( customClasses.length ) { + blockAttributes.className = customClasses.join( ' ' ); } else { delete blockAttributes.className; } diff --git a/packages/editor/src/hooks/test/custom-class-name.js b/packages/editor/src/hooks/test/custom-class-name.js index c7f1b3e1c1c5c5..ea7b441de8459e 100644 --- a/packages/editor/src/hooks/test/custom-class-name.js +++ b/packages/editor/src/hooks/test/custom-class-name.js @@ -10,7 +10,7 @@ import { getHTMLRootElementClasses } from '../custom-class-name'; describe( 'custom className', () => { const blockSettings = { - save: () =>
, + save: () =>
, category: 'common', title: 'block title', }; @@ -95,7 +95,7 @@ describe( 'custom className', () => { const attributes = addParsedDifference( { className: 'foo' }, blockSettings, - '
' + '
' ); expect( attributes.className ).toBe( 'foo bar baz' ); @@ -115,7 +115,7 @@ describe( 'custom className', () => { const attributes = addParsedDifference( {}, blockSettings, - '
' + '
' ); expect( attributes.className ).toBe( 'foo' ); @@ -123,12 +123,12 @@ describe( 'custom className', () => { it( 'should remove the custom class and retain default class', () => { const attributes = addParsedDifference( - { className: 'default custom' }, + { className: 'custom1 custom2' }, blockSettings, - '
' + '
' ); - expect( attributes.className ).toBe( 'default' ); + expect( attributes.className ).toBe( 'custom1' ); } ); it( 'should remove the custom class from an element originally without a class', () => { @@ -143,12 +143,12 @@ describe( 'custom className', () => { it( 'should remove the custom classes and retain default and other custom class', () => { const attributes = addParsedDifference( - { className: 'default custom1 custom2 custom3' }, + { className: 'custom1 custom2 custom3' }, blockSettings, '
' ); - expect( attributes.className ).toBe( 'default custom1 custom3' ); + expect( attributes.className ).toBe( 'custom1 custom3' ); } ); } ); } );