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

add color support to separator block #16784

Merged
merged 7 commits into from
Jul 31, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
10 changes: 9 additions & 1 deletion packages/block-library/src/separator/block.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
{
"name": "core/separator",
"category": "layout"
"category": "layout",
"attributes": {
"color": {
"type": "string"
},
"customColor": {
"type": "string"
}
}
}
45 changes: 43 additions & 2 deletions packages/block-library/src/separator/edit.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,49 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { HorizontalRule } from '@wordpress/components';
import {
InspectorControls,
withColors,
PanelColorSettings,
} from '@wordpress/block-editor';

export default function SeparatorEdit( { className } ) {
return <HorizontalRule className={ className } />;
function SeparatorEdit( { color, setColor, className } ) {
return (
<>
<HorizontalRule
className={ classnames(
className, {
'has-background': color.color,
[ color.class ]: color.class,
}
) }
style={ {
borderColor: color.color,
color: color.color,
} }
/>
<InspectorControls>
<PanelColorSettings
title={ __( 'Color Settings' ) }
colorSettings={ [
{
value: color.color,
onChange: setColor,
label: __( 'Color' ),
},
] }
>
</PanelColorSettings>
</InspectorControls>
</>
);
}

export default withColors( 'color', { textColor: 'color' } )( SeparatorEdit );
42 changes: 40 additions & 2 deletions packages/block-library/src/separator/save.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,41 @@
export default function save() {
return <hr />;
/**
* External dependencies
*/
import classnames from 'classnames';

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

export default function separatorSave( { 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 separatorClasses = classnames( {
'has-text-color has-background': color || customColor,
[ backgroundClass ]: backgroundClass,
[ colorClass ]: colorClass,
} );

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

return ( <hr
className={ separatorClasses }
style={ separatorStyle }
/> );
}
6 changes: 4 additions & 2 deletions packages/block-library/src/separator/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@

// Dots style
&.is-style-dots {
background: none; // Override any background themes often set on the hr tag for this style.
// Override any background themes often set on the hr tag for this style.
// also override the color set in the editor since it's intented for normal HR
background: none !important;
Comment on lines +11 to +13
Copy link
Member

Choose a reason for hiding this comment

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

I see there was some discussion about the use of !important here in the pull request comments, and separately a fix addressing a TwentyTwenty-specific incompatibility at Trac#47811.

Which leaves the question: Do we actually need it? Can it be removed?

See also: #19539

Alternatively, I wonder:

  • Only including this specific style rule if .has-text-color and/or .has-background, representative of the fact that the default should only need to be reset when the user chooses to apply a custom color (correct me if I'm mistaken)
  • Can it be addressed by assuring that the specificity of this selector would always "defeat" a theme's styling? Especially considering we would probably want this reset only in the case of applying when a custom color is used, i.e. adding .has-text-color and/or .has-background as additional modifier classes:
.wp-block-separator.is-style-dots.has-background,
.wp-block-separator.is-style-dots.has-text-color {
	background: none;
}

Choose a reason for hiding this comment

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

What if a different method for generating dots was used?
The current method is a lot of CSS, that is a hassle for themes to override. I was going for the minimal style of making dots with multiple background images using radial gradients, but the current style using !important wouldn't let me.
I also couldn't use a dotted border because all those other styles I would have to override.

So what if the core style is the radial gradient instead of an important no background?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be worth exploring a simpler approach.

border: none;
text-align: center;
max-width: none;
Expand All @@ -17,7 +19,7 @@

&::before {
content: "\00b7 \00b7 \00b7";
color: $dark-gray-900;
color: currentColor;
font-size: 20px;
letter-spacing: 2em;
padding-left: 2em;
Expand Down