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

Separator block: refactor to use color block supports #38428

Merged
merged 26 commits into from
Mar 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
32eb898
Modify edit and save methods of separator to use color block supports
Feb 28, 2022
e59364e
Fix the editor code
Feb 3, 2022
3452d35
Add deprecation
Feb 4, 2022
c914b84
Fix issue with incorrect background color for dotted style in editor
Feb 4, 2022
d9dfdb1
Add new fixtures
Feb 4, 2022
946a00b
Only apply default opacity style for v1 of block
Feb 7, 2022
72bc595
Add an opacity attribute so that the deprecated blocks keep the defau…
Feb 28, 2022
620a2be
Typo fix
glendaviesnz Feb 11, 2022
6cc4459
Update to ensure deprecated opacity is also applied on blocks with no…
Feb 11, 2022
61672a5
Removing fixture JSON props pursuant to https://github.com/WordPress/…
ramonjd Feb 14, 2022
bc6882e
Fix e2e test
Feb 22, 2022
e8d02a1
Another e2e test fix
Feb 23, 2022
c6ad0db
Move deprecated css to a separate file
Feb 23, 2022
0a73535
Add background color default
Feb 23, 2022
bd6f912
Set default attribute to simplify the migration of old blocks
Feb 24, 2022
6457a8a
Extract the deprecated opacity settings to a custom hook
Feb 27, 2022
ba46d5e
Tidy up application of inline styles and add comment to deprecated op…
Feb 28, 2022
e5cbdef
Add another explanatory comment.
Feb 28, 2022
245edc8
Add initial edit method unit test
Feb 28, 2022
9248a09
Regenerate fixtures to remove default attributes
Feb 28, 2022
bc2cacd
Fix unit tests
Feb 28, 2022
ea1b9b1
Some more test fixes
Mar 1, 2022
20cf8f1
Hopefully the last test fix
Mar 1, 2022
91487e0
Add some more tests
Mar 2, 2022
e4ef3d3
Switch to skipSerialization
Mar 16, 2022
22ff3a2
Revert inline styles so they are the same as the existing block to av…
Mar 16, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/reference-guides/core-blocks.md
Original file line number Diff line number Diff line change
Expand Up @@ -698,8 +698,8 @@ Create a break between ideas or sections with a horizontal separator. ([Source](

- **Name:** core/separator
- **Category:** design
- **Supports:** align (center, full, wide), anchor
- **Attributes:** color, customColor
- **Supports:** align (center, full, wide), anchor, color (background, gradients, ~~text~~)
- **Attributes:** opacity

## Shortcode

Expand Down
19 changes: 13 additions & 6 deletions packages/block-library/src/separator/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,23 @@
"keywords": [ "horizontal-line", "hr", "divider" ],
"textdomain": "default",
"attributes": {
"color": {
"type": "string"
},
"customColor": {
"type": "string"
"opacity": {
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
"type": "string",
"default": "alpha-channel"
}
},
"supports": {
"anchor": true,
"align": [ "center", "wide", "full" ]
"align": [ "center", "wide", "full" ],
"color": {
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
"__experimentalSkipSerialization": true,
"gradients": true,
"background": true,
"text": false,
"__experimentalDefaultControls": {
"background": true
}
}
},
"styles": [
{ "name": "default", "label": "Default", "isDefault": true },
Expand Down
57 changes: 57 additions & 0 deletions packages/block-library/src/separator/deprecated.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* External dependencies
*/
import classnames from 'classnames';
import { omit } from 'lodash';

/**
* WordPress dependencies
*/
import { getColorClassName, useBlockProps } from '@wordpress/block-editor';

const v1 = {
attributes: {
color: {
type: 'string',
},
customColor: {
type: 'string',
},
},
save( { attributes } ) {
const { color, customColor } = attributes;

// the hr support changing color using border-color, since border-color
// is not yet supported in the color palette, we use background-color
const backgroundClass = getColorClassName( 'background-color', color );
// the dots styles uses text for the dots, to change those dots color is
// using color, not backgroundColor
const colorClass = getColorClassName( 'color', color );

const className = classnames( {
'has-text-color has-background': color || customColor,
[ backgroundClass ]: backgroundClass,
[ colorClass ]: colorClass,
} );

const style = {
backgroundColor: backgroundClass ? undefined : customColor,
color: colorClass ? undefined : customColor,
};

return <hr { ...useBlockProps.save( { className, style } ) } />;
},
migrate( attributes ) {
const { color, customColor } = attributes;
return {
...omit( attributes, [ 'color', 'customColor' ] ),
backgroundColor: color ? color : undefined,
opacity: 'css',
style: customColor
? { color: { background: customColor } }
: undefined,
};
},
};

export default [ v1 ];
6 changes: 6 additions & 0 deletions packages/block-library/src/separator/deprecated.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.wp-block-separator {
// V1 version of the block expects a default opactiy of 0.4 to be set.
&.has-css-opacity {
opacity: 0.4;
}
Copy link
Member

@ramonjd ramonjd Feb 23, 2022

Choose a reason for hiding this comment

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

I like externalizing the deprecated styles. Hope it becomes a convention. Much clearer to read.

}
50 changes: 36 additions & 14 deletions packages/block-library/src/separator/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,52 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { HorizontalRule } from '@wordpress/components';
import { withColors, useBlockProps } from '@wordpress/block-editor';
import {
useBlockProps,
getColorClassName,
__experimentalUseColorProps as useColorProps,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import SeparatorSettings from './separator-settings';
import useDeprecatedOpacity from './use-deprecated-opacity';

export default function SeparatorEdit( { attributes, setAttributes } ) {
const { backgroundColor, opacity, style } = attributes;
const colorProps = useColorProps( attributes );
const currentColor = colorProps?.style?.backgroundColor;
const hasCustomColor = !! style?.color?.background;

useDeprecatedOpacity( opacity, currentColor, setAttributes );

// The dots styles uses text for the dots, to change those dots color is
// using color, not backgroundColor.
const colorClass = getColorClassName( 'color', backgroundColor );

const className = classnames(
{
'has-text-color': backgroundColor || currentColor,
[ colorClass ]: colorClass,
'has-css-opacity': opacity === 'css',
'has-alpha-channel-opacity': opacity === 'alpha-channel',
},
colorProps.classname
);

const styles = {
color: currentColor,
backgroundColor: currentColor,
};

function SeparatorEdit( { color, setColor, className } ) {
return (
<>
<HorizontalRule
{ ...useBlockProps( {
className: classnames( className, {
'has-background': color.color,
[ color.class ]: color.class,
} ),
style: {
backgroundColor: color.color,
color: color.color,
},
className,
style: hasCustomColor ? styles : undefined,
} ) }
/>
<SeparatorSettings color={ color } setColor={ setColor } />
</>
);
}

export default withColors( 'color', { textColor: 'color' } )( SeparatorEdit );
6 changes: 6 additions & 0 deletions packages/block-library/src/separator/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,10 @@
// Prevent margin collapsing so the area to select the separator is bigger.
padding-top: 0.1px;
padding-bottom: 0.1px;

// This is also set in style.scss, but needs a higher specificity in editor
// due to the way that color block supports adds additional background color styles.
&.wp-block-separator.is-style-dots {
background: none !important;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to deal with the specificity issue other than using !important here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a closer look next week and see that is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oandregal I looked into this and couldn't see a way around it as the color supports is already adding ! important in other places:

Screen Shot 2022-02-28 at 2 51 06 PM

The only potential option I could see is if we could remove the backGround support when is-style-dots is selected and just apply color instead, but this would remove the has-background* classes which could break backwards compat as all the existing blocks have both text and background classes applied, and themes may be relying on these.

Copy link
Member

Choose a reason for hiding this comment

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

The issue seems to be that this block does things a bit differently depending on the kind of style variation applied. As I understand it, we don't need the automatic classes (.has-<slug>-color, etc) attached to the block wrapper in all the states of this block (when the dot style variation is applied).

How about using __experimentalSkipSerialization in the color block supports of the block.json and removing the !importants? I see that this PR is already doing some custom things in the edit and save functions, as well as the block stylesheet. When we do so, is because the normal default block support mechanism is not working as we want, so should look at disconnecting the automatic style serialization.

Copy link
Member

Choose a reason for hiding this comment

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

How about using __experimentalSkipSerialization in the color block supports of the block.json and removing the !importants?

It would be good to get this PR in: Block Supports: Allow skipping serialization of individual features #36293.

That way we can choose specific block supports to "skip". Might mean less work, that not having to manually apply attributes.styles, later if we add block supports to the Separator.

Copy link
Contributor Author

@glendaviesnz glendaviesnz Feb 28, 2022

Choose a reason for hiding this comment

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

The issue seems to be that this block does things a bit differently depending on the kind of style variation applied.
As I understand it, we don't need the automatic classes (.has--color, etc) attached to the block wrapper in
all the states of this block (when the dot style variation is applied).

@oandregal yes, ideally the .has-<slug>-color classes should just be applied when the block is in the relevant state, eg. background-color when it is standard and color when it is dotted, in which case skipping serialization would make sense.

Unfortunately, the block currently applies both classes in both instances, and we have no idea how theme authors might be relying on this current combination of selectors. This means that moving to using the single selector in the specific instances would be a breaking change. I am not sure that the value gained from this change would be worth the potential impact on themes?

Also, the background-color: none ! important is still going to be needed in order to make sure that theme background-color is overridden when the dotted style is selected as noted here. This has always been included in the block, this additional setting was only added to make sure it was still applied in the editor with the addition of the new color supports settings.

For the above reasons I think we should leave the classes are they currently are and leave the background-color: none ! important in place, but open to suggestions for ways that we can remove it while avoiding a breaking change, or also open to feedback on whether this change is important enough to warrant a breaking change.

Copy link
Member

@ramonjd ramonjd Feb 28, 2022

Choose a reason for hiding this comment

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

This is a hard one.

The background classes don't make sense on the dots style, but yeah... they're there ,and some folks might be counting on that. Avoiding pain for users and theme developers seems like it should take priority.

Could we deprecate and migrate in a follow up PR?

On a higher level, we've chosen, for whatever reasons^, to use !important in various places to ensure users' style choices take precedence.

I think the struggle is how to prioritize these preferences, in this case a user-elected style vs a chosen background color. The question might be, "which !important is more important here"? Does that sound reasonable?

We probably have to deal with it as best as we can until we find a way to mitigate such situations.

^ Edit: I got a bit wiser after reading some of the context

Copy link
Member

@oandregal oandregal Mar 11, 2022

Choose a reason for hiding this comment

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

re: having classes pertinent to the state.

Yeah, I understand the concerns you've brought up: that change alone would need wider testing in themes. The original PR didn't clarify why it added the both classes in every state. It's fine by me to leave it as it is in this PR and explore its removal in a follow up.

Regarding the other thing I've mentioned: I still think we need to use __experimentalSkipSerialization in the color support since we're already managing the serialization by the block.

Copy link
Contributor Author

@glendaviesnz glendaviesnz Mar 16, 2022

Choose a reason for hiding this comment

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

Regarding the other thing I've mentioned: I still think we need to use __experimentalSkipSerialization in the color support since we're already managing the serialization by the block.

Good point @oandregal, doing that makes it a bit clearer that this isn't a fully standard use of the color supports even though the background colors are being applied correctly via serialization. I have made that change now.

}
}
2 changes: 2 additions & 0 deletions packages/block-library/src/separator/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import edit from './edit';
import metadata from './block.json';
import save from './save';
import transforms from './transforms';
import deprecated from './deprecated';

const { name } = metadata;

Expand All @@ -26,4 +27,5 @@ export const settings = {
transforms,
edit,
save,
deprecated,
};
36 changes: 22 additions & 14 deletions packages/block-library/src/separator/save.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,36 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { getColorClassName, useBlockProps } from '@wordpress/block-editor';
import {
getColorClassName,
useBlockProps,
__experimentalGetColorClassesAndStyles as getColorClassesAndStyles,
} from '@wordpress/block-editor';

export default function separatorSave( { attributes } ) {
const { color, customColor } = attributes;

const { backgroundColor, style, opacity } = attributes;
const customColor = style?.color?.background;
const colorProps = getColorClassesAndStyles( attributes );
// The hr support changing color using border-color, since border-color
// is not yet supported in the color palette, we use background-color.
const backgroundClass = getColorClassName( 'background-color', color );

// The dots styles uses text for the dots, to change those dots color is
// using color, not backgroundColor.
const colorClass = getColorClassName( 'color', color );
const colorClass = getColorClassName( 'color', backgroundColor );

const className = classnames( {
'has-text-color has-background': color || customColor,
[ backgroundClass ]: backgroundClass,
[ colorClass ]: colorClass,
} );
const className = classnames(
{
'has-text-color': backgroundColor || customColor,
[ colorClass ]: colorClass,
'has-css-opacity': opacity === 'css',
'has-alpha-channel-opacity': opacity === 'alpha-channel',
},
colorProps.className
);

const style = {
backgroundColor: backgroundClass ? undefined : customColor,
const styles = {
backgroundColor: colorProps?.style?.backgroundColor,
color: colorClass ? undefined : customColor,
};

return <hr { ...useBlockProps.save( { className, style } ) } />;
return <hr { ...useBlockProps.save( { className, style: styles } ) } />;
}
24 changes: 0 additions & 24 deletions packages/block-library/src/separator/separator-settings.js

This file was deleted.

113 changes: 113 additions & 0 deletions packages/block-library/src/separator/test/edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';

/**
* WordPress dependencies
*/
import { useBlockProps } from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import SeparatorEdit from '../edit';

jest.mock( '@wordpress/block-editor', () => ( {
...jest.requireActual( '@wordpress/block-editor' ),
useBlockProps: jest.fn(),
} ) );

const defaultAttributes = {
backgroundColor: undefined,
opacity: 'alpha-channel',
style: {},
className: '',
};
const defaultProps = {
attributes: defaultAttributes,
setAttributes: jest.fn(),
};

describe( 'Separator block edit method', () => {
beforeEach( () => {
useBlockProps.mockClear();
} );

test( 'should add the has-alpha-channel-opacity class and no inline styles by default', () => {
render( <SeparatorEdit { ...defaultProps } /> );
expect( useBlockProps ).toHaveBeenCalledWith( {
className: 'has-alpha-channel-opacity',
style: undefined,
} );
} );

test( 'should add has-css-opacity class and no inline styles for deprecated block with no color specified', () => {
const props = {
...defaultProps,
attributes: { ...defaultAttributes, opacity: 'css' },
};
render( <SeparatorEdit { ...props } /> );
expect( useBlockProps ).toHaveBeenCalledWith( {
className: 'has-css-opacity',
style: undefined,
} );
} );

test( 'should add inline background style for block without dots style selected and custom color specified', () => {
const props = {
...defaultProps,
attributes: {
...defaultAttributes,
style: { color: { background: '#ff0000' } },
},
};
render( <SeparatorEdit { ...props } /> );
expect( useBlockProps ).toHaveBeenCalledWith( {
// For backwards compatibility the has-text-color class is also added even though it is only needed for
// is-style-dots as this class was always added to v1 blocks, so may be expected by themes and plugins.
className: 'has-text-color has-alpha-channel-opacity',
style: {
backgroundColor: '#ff0000',
color: '#ff0000',
},
} );
} );

test( 'should add inline color style for block with dots style selected and custom color specified', () => {
const props = {
...defaultProps,
attributes: {
...defaultAttributes,
className: 'is-style-dots',
style: { color: { background: '#ff0000' } },
},
};
render( <SeparatorEdit { ...props } /> );
expect( useBlockProps ).toHaveBeenCalledWith( {
className: 'has-text-color has-alpha-channel-opacity',
style: {
backgroundColor: '#ff0000',
color: '#ff0000',
},
} );
} );

test( 'should add color class when color from palette specified', () => {
const props = {
...defaultProps,
attributes: {
...defaultAttributes,
backgroundColor: 'banana',
},
};
render( <SeparatorEdit { ...props } /> );
// Note that only the manual addition of the text color class can be checked as the
// background color classes are added by useBlockProps which has to be mocked.
expect( useBlockProps ).toHaveBeenCalledWith( {
className:
'has-text-color has-banana-color has-alpha-channel-opacity',
style: undefined,
} );
} );
} );
Loading