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 4 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
2 changes: 1 addition & 1 deletion packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function GroupEdit( { className, setBackgroundColor, backgroundColor } ) {
/>
</InspectorControls>
<div className={ classes } style={ styles }>
<InnerBlocks />
<InnerBlocks renderAppender={ () => <InnerBlocks.ButtonBlockAppender /> } />
Copy link
Contributor

@youknowriad youknowriad Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @talldan for the quick PR, in testing I wonder if the appender should be hidden if the block is unselected and already contain items, what do people think? the condition could be

( isSelected || ! hasChildren ) && <Appender />

What do you think @mapk @jasmussen @mtias

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @talldan for the quick PR, in testing I wonder if the appender should be hidden if the block is unselected and already contain items, what do people think? the condition could be

( isSelected || ! hasChildren ) && <Appender />

What do you think @mapk @jasmussen @mtias

This needs a decision. I'd tend to agree with @youknowriad that I was surprised by how out-of-place the button inserter feels after having inserted an "initial" block (i.e. once it's no longer empty).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it makes sense to hide it under those conditions. That helps preserve the 1:1 visual relationship to the front end. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Related: #14241 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it makes sense to hide it under those conditions. That helps preserve the 1:1 visual relationship to the front end. 👍

Hmm, I've been experimenting with this. I can't quite put a finger on it, but something about it feels not quite right. It's notably hard to follow when and why the appender is visible if the Group does not have a background assigned, or there are Group blocks within or adjacent to one another (similar observation as #14943 (comment)).

You can give it a try at: http://gutenberg.run/14966
Branch: #14966

Copy link
Contributor

Choose a reason for hiding this comment

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

Great thoughts, thanks for trying that branch, Andrew, and what is this wonderful new gutenberg.run thing!? I love it. Here's what i see:

group

My very first instinct is that as soon as you've inserted a single block inside the group, we no longer need to use the generic appender for that block. That interface only feels necessary for the setup state. After that, the same interface that every other block uses — press enter or use the sibling inserter, feels sufficient to me. We already have so many many (+) buttons all over the place, we really need to be careful in adding more of them.

What do you think?

</div>
</Fragment>
);
Expand Down
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 -->"
`;
17 changes: 15 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,21 @@ 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.waitForSelector( '.block-editor-button-block-appender' );
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit concerning to me that we have to wait. Why isn't it immediately available?

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 think I'm feeling a little bit burned by failing tests and go with the safest option. 😄

I've removed those calls and tests still pass.

await page.click( '.block-editor-button-block-appender' );

await page.waitForSelector( '.editor-block-list-item-paragraph' );
await page.click( '.editor-block-list-item-paragraph' );

await page.keyboard.type( 'Group Block with a Paragraph' );

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