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.

Sign up for GitHub

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

Use button block appender on group block #14943

Merged
merged 6 commits into from
Apr 30, 2019
Merged

Use button block appender on group block #14943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c965e5
Use button block appender on group block
talldan Apr 12, 2019
be263c3
Update group block e2e tests for the new button appender
talldan Apr 12, 2019
926b2bf
Add additional margin to the appender when a group has a background c…
kjellr Apr 12, 2019
473d62d
Use more opaque versions of the appender background color
kjellr Apr 12, 2019
c5f27fa
Block Library: Display ButtonBlockAppender only if selected or empty
aduth Apr 12, 2019
443822c
Only render button appender when the group has no inner blocks
talldan Apr 18, 2019
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
2 changes: 2 additions & 0 deletions assets/stylesheets/_colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ $dark-opacity-light-400: rgba(#95959c, 0.2);
$dark-opacity-light-300: rgba(#829493, 0.15);
$dark-opacity-light-200: rgba(#8b8b96, 0.1);
$dark-opacity-light-100: rgba(#747474, 0.05);
$dark-opacity-background-fill: rgba($dark-gray-700, 0.7); // Similar to $dark-opacity-light-200, but more opaque.

// Light opacities, for use with dark themes.
$light-opacity-900: rgba($white, 1);
Expand All @@ -64,6 +65,7 @@ $light-opacity-light-400: rgba($white, 0.25);
$light-opacity-light-300: rgba($white, 0.2);
$light-opacity-light-200: rgba($white, 0.15);
$light-opacity-light-100: rgba($white, 0.1);
$light-opacity-background-fill: rgba($light-gray-300, 0.8); // Similar to $light-opacity-light-200, but more opaque.

// Additional colors.
// Some are from https://make.wordpress.org/design/handbook/foundations/colors/.
Expand Down
6 changes: 6 additions & 0 deletions packages/block-editor/src/components/block-list-appender/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
.block-list-appender {
margin: $block-padding;

// Add additional margin to the appender when inside a group with a background color.
// If changing this, be sure to sync up with group/editor.scss line 13.
.has-background & {
margin: ($block-padding*2 + $block-spacing) $block-padding;
}
}

.block-list-appender > .block-editor-inserter {
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/button-block-appender/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
outline: $border-width dashed $dark-gray-150;
width: 100%;
color: $dark-gray-500;
background: $dark-opacity-light-100;
background: $light-opacity-background-fill;

&:hover,
&:focus {
Expand All @@ -22,7 +22,7 @@

// Use opacity to work in various editor styles
.is-dark-theme & {
background: $light-opacity-light-100;
background: $dark-opacity-background-fill;
color: $light-gray-100;

&:hover,
Expand Down
30 changes: 27 additions & 3 deletions packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { Fragment } from '@wordpress/element';
import { withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import {
InspectorControls,
Expand All @@ -15,7 +17,14 @@ import {
withColors,
} from '@wordpress/block-editor';

function GroupEdit( { className, setBackgroundColor, backgroundColor } ) {
const renderAppender = () => <InnerBlocks.ButtonBlockAppender />;

function GroupEdit( {
className,
setBackgroundColor,
backgroundColor,
hasInnerBlocks,
} ) {
const styles = {
backgroundColor: backgroundColor.color,
};
Expand All @@ -39,10 +48,25 @@ function GroupEdit( { className, setBackgroundColor, backgroundColor } ) {
/>
</InspectorControls>
<div className={ classes } style={ styles }>
<InnerBlocks />
<InnerBlocks
renderAppender={ ! hasInnerBlocks && renderAppender }
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind to have seen this function be inlined.

Alternatively (separately), I would like to see as proposed in #14241 (comment) with createElement being used with renderAppender so this could be come:

renderAppender={ ! hasInnerBlocks && InnerBlocks.ButtonBlockAppender }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I'll follow up with a PR that tries out createElement. The only concern I'd have is lack of consistency with other render props, but that would only be an issue if renderAppender ever took arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is here - #15259

/>
</div>
</Fragment>
);
}

export default withColors( 'backgroundColor' )( GroupEdit );
export default compose( [
withColors( 'backgroundColor' ),
withSelect( ( select, { clientId } ) => {
const {
getBlock,
} = select( 'core/block-editor' );

const block = getBlock( clientId );

return {
hasInnerBlocks: !! ( block && block.innerBlocks.length ),
};
} ),
] )( GroupEdit );
12 changes: 8 additions & 4 deletions packages/e2e-tests/specs/blocks/__snapshots__/group.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,20 @@

exports[`Group can be created using the block inserter 1`] = `
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>Group block</p>
<!-- /wp:paragraph --></div>
<div class=\\"wp-block-group\\"></div>
<!-- /wp:group -->"
`;

exports[`Group can be created using the slash inserter 1`] = `
"<!-- wp:group -->
<div class=\\"wp-block-group\\"></div>
<!-- /wp:group -->"
`;

exports[`Group can have other blocks appended to it using the button appender 1`] = `
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:paragraph -->
<p>Group block</p>
<p>Group Block with a Paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->"
`;
12 changes: 10 additions & 2 deletions packages/e2e-tests/specs/blocks/group.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ describe( 'Group', () => {
it( 'can be created using the block inserter', async () => {
await searchForBlock( 'Group' );
await page.click( '.editor-block-list-item-group' );
await page.keyboard.type( 'Group block' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
Expand All @@ -25,7 +24,16 @@ describe( 'Group', () => {
await clickBlockAppender();
Copy link
Member

Choose a reason for hiding this comment

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

It wasn't introduced here, so not necessary to change, but I find that testing the block inserter and the slash inserter as distinct tasks is not something which should be done in tests for individual blocks, but rather specific to those inserter interactions. This could save us a fair amount of effort (and build runtime) to avoid running them for every combination of blocks.

await page.keyboard.type( '/group' );
await page.keyboard.press( 'Enter' );
await page.keyboard.type( 'Group block' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );

it( 'can have other blocks appended to it using the button appender', async () => {
await searchForBlock( 'Group' );
await page.click( '.editor-block-list-item-group' );
await page.click( '.block-editor-button-block-appender' );
await page.click( '.editor-block-list-item-paragraph' );
await page.keyboard.type( 'Group Block with a Paragraph' );

expect( await getEditedPostContent() ).toMatchSnapshot();
} );
Expand Down