-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 initial draft of container block #10562
Changes from 4 commits
3cee715
4a46fdc
83a9704
dee2ba2
872d6fe
b524cd5
5acc4e6
3d33685
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
.wp-block-container .editor-block-list__block { | ||
// We need to enforce a cascade by telling nested blocks to inherit | ||
// their text color from the container block. | ||
color: inherit; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import classnames from 'classnames'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { withFallbackStyles } from '@wordpress/components'; | ||
import { compose } from '@wordpress/compose'; | ||
import { __, sprintf } from '@wordpress/i18n'; | ||
import { | ||
getColorClassName, | ||
withColors, | ||
ContrastChecker, | ||
InnerBlocks, | ||
InspectorControls, | ||
PanelColorSettings, | ||
} from '@wordpress/editor'; | ||
import { Fragment } from '@wordpress/element'; | ||
|
||
const applyFallbackStyles = withFallbackStyles( ( node, ownProps ) => { | ||
const { textColor, backgroundColor } = ownProps.attributes; | ||
|
||
return { | ||
fallbackBackgroundColor: backgroundColor, | ||
fallbackTextColor: textColor, | ||
}; | ||
} ); | ||
|
||
export const name = 'core/container'; | ||
|
||
export const settings = { | ||
title: sprintf( | ||
/* translators: Block title modifier */ | ||
__( '%1$s (%2$s)' ), | ||
__( 'Container' ), | ||
__( 'beta' ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the UI freeze a block freeze? In which case, I don't think we should want anything "beta"-denoted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch 👍 |
||
), | ||
|
||
icon: <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M19 12h-2v3h-3v2h5v-5zM7 9h3V7H5v5h2V9zm14-6H3c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h18c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm0 16.01H3V4.99h18v14.02z" /><path d="M0 0h24v24H0z" fill="none" /></svg>, | ||
|
||
category: 'layout', | ||
|
||
attributes: { | ||
align: { | ||
type: 'string', | ||
}, | ||
backgroundColor: { | ||
type: 'string', | ||
}, | ||
customBackgroundColor: { | ||
type: 'string', | ||
}, | ||
textColor: { | ||
type: 'string', | ||
}, | ||
customTextColor: { | ||
type: 'string', | ||
}, | ||
}, | ||
|
||
description: __( 'Group blocks into a container.' ), | ||
|
||
supports: { | ||
align: [ 'wide', 'full' ], | ||
anchor: true, | ||
}, | ||
|
||
edit: compose( [ | ||
withColors( 'backgroundColor', { textColor: 'color' } ), | ||
applyFallbackStyles, | ||
] )( ( props ) => { | ||
const { | ||
backgroundColor, | ||
className, | ||
fallbackBackgroundColor, | ||
fallbackTextColor, | ||
setBackgroundColor, | ||
setTextColor, | ||
textColor, | ||
} = props; | ||
|
||
return ( | ||
<Fragment> | ||
<InspectorControls> | ||
<PanelColorSettings | ||
title={ __( 'Color Settings' ) } | ||
initialOpen={ false } | ||
colorSettings={ [ | ||
{ | ||
value: backgroundColor.color, | ||
onChange: setBackgroundColor, | ||
label: __( 'Background Color' ), | ||
}, | ||
{ | ||
value: textColor.color, | ||
onChange: setTextColor, | ||
label: __( 'Text Color' ), | ||
}, | ||
] } | ||
> | ||
<ContrastChecker | ||
{ ...{ | ||
textColor: textColor.color, | ||
backgroundColor: backgroundColor.color, | ||
fallbackTextColor, | ||
fallbackBackgroundColor, | ||
} } | ||
/> | ||
</PanelColorSettings> | ||
</InspectorControls> | ||
<div | ||
className={ classnames( className, { | ||
'has-background': backgroundColor.color, | ||
'has-text-color': textColor.color, | ||
[ backgroundColor.class ]: backgroundColor.class, | ||
[ textColor.class ]: textColor.class, | ||
} ) } | ||
style={ { | ||
backgroundColor: backgroundColor.color, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the inline styles be necessary? Why isn't it enough to assign the color classes? (Maybe this is a known limitation, but at a high-level it feels wrong) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduth if someone picks a custom color, it has to be applied via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So then what's the purpose of the class name being assigned? Again, not particularly directed at you or this block, maybe more a general issue we have with colors assignment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduth If you pick one of the colors in the palette, it assigns a class. If you pick a color outside the palette, it applies a style. This is the rendered HTML for a block using the default red/light gray colors: <div class="wp-block-container has-background has-text-color has-vivid-red-background-color has-very-light-gray-color">
<p>Container, Paragraph One</p>
<p>Container, Paragraph Two</p>
</div> Vs this rendered HTML from the same block using two custom colors selected via the color picker popover: <div class="wp-block-container has-background has-text-color" style="background-color:#2eaacf;color:#ffffff">
<p>Container, Paragraph One</p>
<p>Container, Paragraph Two</p>
</div> I'm not quite sure how this magic works, but it does 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduth Oh I see you're referring to the edit representation of the block, not the front-end version, and that's a great question. I'll take a closer look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pushing up a commit to fix this. I can open a separate PR to do this for the paragraph block as well. |
||
color: textColor.color, | ||
} } | ||
> | ||
<InnerBlocks /> | ||
</div> | ||
</Fragment> | ||
); | ||
} ), | ||
|
||
save( { attributes } ) { | ||
const { | ||
backgroundColor, | ||
customBackgroundColor, | ||
customTextColor, | ||
textColor, | ||
} = attributes; | ||
|
||
const backgroundColorClass = getColorClassName( 'background-color', backgroundColor ); | ||
const textColorClass = getColorClassName( 'color', textColor ); | ||
|
||
return ( | ||
<div | ||
className={ classnames( { | ||
'has-background': backgroundColor || customBackgroundColor, | ||
'has-text-color': textColor || customTextColor, | ||
[ backgroundColorClass ]: backgroundColorClass, | ||
[ textColorClass ]: textColorClass, | ||
} ) } | ||
style={ { | ||
backgroundColor: backgroundColorClass ? undefined : customBackgroundColor, | ||
color: textColorClass ? undefined : customTextColor, | ||
} } | ||
> | ||
<InnerBlocks.Content /> | ||
</div> | ||
); | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
.wp-block-container { | ||
// 1px top/bottom padding allows us to prevent margin collapsing while | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does margin collapse happen on the front-end, or is this meant to offset the editor styles? If the latter, should we then include this only in an editor-specific stylesheet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Front and back-end, but in my testing it was good/necessary for both cases. |
||
// avoiding excessive top/bottom margins. | ||
padding: 1px 1em; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is there a color otherwise being assigned that is breaking the default cascade? It's not clear to me that this should be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth See my comment here: #10067 (comment)
We map
body
to.editor-block-list__block
insideeditor-styles.scss
which means each block resets the cascade for these properties.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit unfortunate 😕 But OK 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed we should refactor the editor styles to apply to the editor container. The downside though is that it can affect the UI bits (toolbars, movers...) so we need to be careful there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost wonder if there's a way to do a forced reset for Gutenberg chrome? Something to ponder for the future but for now this works :)