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

Adds Block Appender as placeholder to empty InnerBlocks #14241

Merged

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Mar 5, 2019

When inserting a Block that makes use of InnerBlocks(eg: Columns) there are two issues:

  1. It isn't always clear that the Block had been inserted (few visual cues)
  2. By default an empty paragraph Block is inserted - typically with high level Blocks this isn't what you want and it hides the controls on the parent Block and shows those of the paragraph Block.

This change aims to address this by replacing the default behaviour and instead showing the Block Appender as a placeholder:

screen shot 2019-03-05 at 16 16 24

This works by checking whether the parent Block has 1 or more innerBlocks. If there are none then it displays the Block Appender as a placeholder thereby clearly indicating the Block has been inserted and providing an opportunity for the user to manually select the appropriate child Block to insert.

Once a child Block is inserted then the placeholder is removed.

This is closely tied to the current work on the Container Block which requires exactly the functionality described above to be perceivable to users. Please see #13964 (comment)

Todos / Outstanding Tasks

  • Adjust appender to use 100 version of the gray.
  • Fix to ensure appender is not larger than column content. Attempt to match up with this.
  • Fix e2e tests that broke as a result of the UI / UX change
  • Write new e2e tests to cover the new UI/UX
  • Write e2e tests to verify
    • it works with a custom appender
    • it works with the exposed components
  • Address final feedback points

How has this been tested?

For testing purposes apply the following patch to enable the ButtonBlockAppender on core/seciton and core/column

diff --git a/packages/block-library/src/columns/column.js b/packages/block-library/src/columns/column.js
index a13779e54..3224f2df0 100644
--- a/packages/block-library/src/columns/column.js
+++ b/packages/block-library/src/columns/column.js
@@ -33,6 +33,9 @@ const ColumnEdit = ( { attributes, updateAlignment } ) => {
 			</BlockControls>
 			<InnerBlocks
 				templateLock={ false }
+				renderAppender={ () => (
+					<InnerBlocks.ButtonBlockAppender />
+				) }
 			/>
 		</div>
 	);
diff --git a/packages/block-library/src/section/edit.js b/packages/block-library/src/section/edit.js
index fcf17c40c..bbbd5adfc 100644
--- a/packages/block-library/src/section/edit.js
+++ b/packages/block-library/src/section/edit.js
@@ -39,7 +39,11 @@ function SectionEdit( { className, setBackgroundColor, backgroundColor } ) {
 				/>
 			</InspectorControls>
 			<div className={ classes } style={ styles }>
-				<InnerBlocks />
+				<InnerBlocks
+					renderAppender={ () => (
+						<InnerBlocks.ButtonBlockAppender />
+					) }
+				/>
 			</div>
 		</Fragment>
 	);

Screenshots

Types of changes

  • New feature (non-breaking change which adds functionality
  • (Possible) Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@getdave getdave added [Feature] Inserter The main way to insert blocks using the + button in the editing interface Needs Design Feedback Needs general design feedback. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Feedback Issues that relate purely to feedback on a feature that isn't necessarily actionable Needs Technical Feedback Needs testing from a developer perspective. Needs Accessibility Feedback Need input from accessibility labels Mar 5, 2019
@getdave getdave self-assigned this Mar 5, 2019
@talldan
Copy link
Contributor

talldan commented Mar 6, 2019

I think only some blocks would want to implement this (container being the main candidate).

The question is ... how should it be declared? Perhaps as a block setting? Would it live under supports? e.g.

supports: { appender: 'button' }

or

supports: { buttonAppender: true }

@youknowriad
Copy link
Contributor

The InnerBlocks component has a bunch of props to configure it right now, so I think it's fine to keep using a prop for consistency.

@richtabor
Copy link
Member

I've played around with an improved column appender as well for the CoBlocks Row/Column blocks. A bit different UI to look at. 💪

screenflow

@getdave
Copy link
Contributor Author

getdave commented Mar 6, 2019

@youknowriad @talldan To confirm we're saying

  1. This should be opt in via the parent Block which uses InnerBlocks
  2. Opt-in should be via a prop passed to InnerBlocks (as opposed to a new API)

Please see my commits 0e1dd78 and 58c60f0 which address this by making it opt in via a prop on InnerBlocks (documented).

Demo - note that Columns is opting in, whereas Media + Text has not opted in. Both Blocks use InnerBlocks:

screen capture on 2019-03-06 at 12-09-19

Question: is the "prop drilling" ok here? Is there a better way?

@richtabor This does indeed look very similar. Are there any enhancements/learnings you'd like to share to benefit this PR? Much appreciated. @melchoyce did the design on this one I believe.

@richtabor
Copy link
Member

@richtabor This does indeed look very similar. Are there any enhancements/learnings you'd like to share to benefit this PR? Much appreciated.

The most important part here is ensuring that folks understand that they can click the appender to add a block.

Also, I've considered keeping the appender at the bottom of the column block, so that users can still easily add blocks after an initial implementation. I was finding that folks were unsure how to add blocks again, once the appender placeholder had been removed (as blocks were added).

@getdave
Copy link
Contributor Author

getdave commented Mar 6, 2019

@richtabor Thanks for your input.

The most important part here is ensuring that folks understand that they can click the appender to add a block.

I'm hopefull that the wording "Add a Block" should suffice (not intending to be sarcastic here - just an observation). I like the background colour on your version as this enhances the perceiability yet further, but I'll defer to @melchoyce on this one.

I've considered keeping the appender at the bottom of the column block

It's a nice idea and one I think could/should be explored in a follow on PR if that's ok with you?

@jasmussen
Copy link
Contributor

Could "Add Block" be sufficient, as oppposed to "Add a block"? Short and sweet, and it's always good to consider translations that nearly always balloon best intentions.

@melchoyce
Copy link
Contributor

We use rgba(139,139,150,.1) for placeholders; let's try that out here in a couple contexts to see how it works.

@getdave getdave mentioned this pull request Mar 6, 2019
15 tasks
@melchoyce
Copy link
Contributor

Oh, while I'm thinking about it — is there any way to figure out if the appender is on a dark background, and if so, flip the colors over to white for contrast?

@jasmussen
Copy link
Contributor

I think we have an SCSS variable for that opacity color, and be sure to test it in dark editor styles!

@talldan talldan force-pushed the add/inner-blocks-block-appender-as-placeholder branch 2 times, most recently from 83cfc04 to 78b7800 Compare April 12, 2019 06:51
@talldan talldan force-pushed the add/inner-blocks-block-appender-as-placeholder branch from 8a1f503 to 13d243a Compare April 12, 2019 07:20
@talldan
Copy link
Contributor

talldan commented Apr 12, 2019

I've added a few extra commits to address the feedback.

Mostly, I haven't made any major changes, but one thing I'd like to do is move the HideWhenChildBlocks component to a separate PR, since it's one of the few remaining points of discussion, and I don't believe it's needed at this point.

I've removed it from this PR and have a separate branch that still has it.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Let's ship and use it in the section block.

@talldan talldan merged commit db7decd into WordPress:master Apr 12, 2019
@talldan
Copy link
Contributor

talldan commented Apr 12, 2019

Great! Other PRs incoming:

@getdave
Copy link
Contributor Author

getdave commented Apr 12, 2019

@talldan Whoa I barely recognised the PR! Thanks for all your work to get this merged 👍

Could you link to the PRs referenced above?

@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
)

* Adds Block Appender as placeholder to empty InnerBlocks

Previously when inserting a Block that makes use of InnerBlocks (eg: Columns) it wasn’t clear that the Block had been inserted and by default an empty paragraph Block was inserted.

This changes checks for any innerBlocks and if available disables the default Block insertion (ie: paragraph) and displays the Block Appender as a placeholder. Once a Block is inserted then the placeholder is removed.

* Updates to allow InnerBlocks to control it’s placeholder when empty

Previously all Blocks making use of `InnerBlocks` were defaulted to showing the `BlockListAppender` as the placeholder. This update allows `InnerBlocks` to opt-in to this feature. It does however, require some prop drilling under the hood.

Addresses: WordPress#14241 (comment)

* Updates Column Block to opt-in to using Block Appender as placeholder

* Fix edge case where block can be null value

Previously test for presence of child blocks assumed that the block being interogated would always be present. In some circumstances this isn’t true resulting in an error. Fixed to include check for presence of block as if it isn’t there then it cannot have any children (obviously).

* Simplifies wording on Block Appender

* Adds background color to Block Appender

Addresses WordPress#14241 (comment)

* Adds support for dark themes

Resolves WordPress#14241 (comment)

* Adds keyboard focus support for visual cues

* Fixes comment to approved version

Resolves WordPress#14241 (review)

* Adjusts background to use lighter colour variant

Resolves WordPress#14241 (comment)

* Adds consistent spacing around appender

This update creates a consistent visual spacing around the appender itself. To handle the edge case of Blocks acting as pass through Blocks (which have negative margins) we have to double the margin on these Blocks.

* Removes unecessary Fragment usage

Resolves WordPress#14241 (comment)

* Adjusts font weight and space between icon and “Add Block” text

Addresses part of WordPress#14241 (comment)

* Adjusts appender padding to match supplied visual

Addresses visual design as provided in WordPress#14241 (comment)

* Aligns appender with sibling Blocks

* Revert "Aligns appender with sibling Blocks"

This reverts commit 49b03efe22f79dd45564def2a22d0693ae2530e7.

* Updates to use explict and more extenable prop for choosing the placeholder

Previously it was only possible to enable/disable the “Add Block” placeholder. In view of requests to be able to enable yet more states (ie: no Block appender _at all_) the API has been updated to a form which will make it easier to add different placeholders in the future.

* Reverts Column to use default placeholder behaviour

As discuseed, removing this from Column to facilitate merge.

* Removes “is passthrough” edge case for Columns

This reverts commit dc5c3526b1ba10e50a0934d934240307657c174a whilst retaining the consistent spacing around the appender which is still required.

This was reverted because it has been decided that Block List Appender should obey SRP and avoid adding styles to cater of edge case Blocks. Such Block variations should either captured within an API or rather should add overides themselves on an case by case basis.

* Fixes Columns layout edge case for Block Appender

Whilst “block-appender” mode will not be enabled for Columns by default we should cater for this scenario.

* Updates colors for hover and active states

Resolves WordPress#14241 (comment)

* Fix docs to show correct variable type

* Updates to appender rendering to utilise render prop

Previously we were constraining control over the type of appenders that could be created. Be utilising an optional render prop we provide developers with the ability to fully customise the render. However, by creating sensible default enum constants within InnerBlocks we preserve the “simple defaults”.

* Adds default appender as explicit constant

Updates to add default “auto insert” appender behaviour as a constant. This way if it is ever removed as the default we have an explicit constant by which to reference this behaviour by which isn’t tied to the term “default”.

* Adds option to hide appender entirely for InnerBlocks if children present

Creates separate prop for InnerBlocks to optionally hide the appender if child Blocks are present. The lower level components have been adjusted with `hideAppender` prop to selectively enable/disable rendering of the appender.

* Updates documentation for `renderAppender` and `hideAppenderWhenChildren`

* Updates to remove superflous usage of `appender` suffix

* Fixes Markdown usage on README heading

* Fix to ensure default appender render when default block is not active

Previously render was returning nothing when `canInsertDefaultBlock` was false (eg: when paragraph Block is disabled but `auto-insert` is still set as the default placeholder when a container Block is inserted. Fixes e2e test failures.

* Exposes placeholder defaults as components on InnerBlocks

Addresses ideas expressed WordPress#14241 (review)

Also an `HideWhenChildren` component to control rendering in scenario when children are present. Note that control over this functionality is now soley controlled by the render prop and cannot be activated via a simple prop.

* Adds mixins to handle visually hiding content but preserving for screenreaders

For approach see https://a11yproject.com/posts/how-to-hide-content/

* Removes the “Add Block” text from the button appender

Hides text visually but retains it for assistive technologies as we cannot rely on an icon to convey meaning.

Addresses WordPress#14241 (comment)

* Adds Button Block Appender component to DRY up component usage

Previously we had duplicates of the Button appender both within `InnerBlocks` and again within the `BlockListAppender` component. Creates a new standalone  `ButtonBlockAppender` component which is used across both components.

Also removes option to provide enum to `InnerBlocks` `renderAppender` prop. Now only render functions are valid. We still expose x2 default “appenders” on `InnerBlocks` to enable easy usage.

* Removes superfluous enum handling on renderAppender prop

* Minor - updates comments to end in full stops

Addresses WordPress#14241 (comment)

* Extracts `hideWhenChildBlocks` component for use on `InnerBlocks`

* Updates docs with correct props and usage examples

* Add docs for `ButtonBlockAppender` component

* Adds ability to pass custom CSS className to ButtonBlockAppender component

* Fixes e2e test by ensuring correct className is restored on appender when used inside BlockListAppender

* Revert "Adds mixins to handle visually hiding content but preserving for screenreaders"

This reverts commit 1a9a2f80ad780709fc781532352585772eb68257.

* Utilise existing WP Core screen reader text class

* Removes dependency on compose

* Improves naming of aliased component

In order to avoid inline component functions within `compose` components have been moved to their own functional components and then enhanced. However this causes a naming clash between the base component and the one being created and then enhanced within this file. As a result, all duplicates are aliased with the `Base` prefix to allow a component of the same name to be reused within the same file.

* Capitalise component name

* Correct spelling

* Simplifies check for prop

* Remove `utils` directory and flatten

* Simplify the use of if statements in BlockListAppender

* Use classnames utility to concat classes

* Rename class to block-editor-button-block-appender

* Switch to either arrow functions assigned to a const or exported function declarations

* revert unintentional formatting changes

* Remove HideWhenChildBlocks. This will be added on a separate branch

* Tidy up docs
This was referenced Apr 17, 2019
@SchneiderSam
Copy link

Can't found this function in 5.5. confused...

@talldan
Copy link
Contributor

talldan commented May 1, 2019

Hey @SchneiderSam - it's being rolled out to blocks on a case by case basis. At the moment it's only been added to the group block.

It was implemented for the group block on a separate PR, but unfortunately missed 5.5 and 5.6. It should be in 5.7, which is due to be released on the 15th May:
#14943

@websevendev
Copy link

I used the renderAppender prop on <InnerBlocks> to render a button that says Add list item for a custom list block. Previously had to click on the appender button and then select the block from a selection of 1 block, so this prop is great.

How ever the button doesn't work when clicking it while the parent block is inactive, it just activates the block. End up needing to click twice most of the time.

Looking at why the normal appender works with one click, but my custom one doesn't, the difference seems to be <IgnoreNestedEvents>:

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list-appender/index.js#L30-L52

I don't think it's exposed, so I copied the <IgnoreNestedEvents> to my library and wrapped my appender button in it and then it works.

Perhaps expose the <IgnoreNestedEvents> for general use or add a prop to use it?

My code:

(hook)

import IgnoreNestedEvents from '../IgnoreNestedEvents';

export default function useAppender(element, blockName, clientId) {
	const append = useCallback(() => {
		const block = wp.blocks.createBlock(blockName);
		const index = wp.data.select('core/editor').getBlocksByClientId(clientId)[0].innerBlocks.length;
		wp.data.dispatch('core/editor').insertBlock(block, index, clientId);
	}, [blockName, clientId]);

	return () => {
		return (
			<IgnoreNestedEvents childHandledEvents={['onFocus', 'onClick', 'onKeyDown']}>
				<element.type {...element.props} onClick={append} />
			</IgnoreNestedEvents>
		);
	};
};

(used in block edit)

const Edit = ({clientId}) => {
	...
	const appender = useAppender(
		<button>Add list item</button>,
		'custom/listitem',
		clientId
	);
	...
	return (
		...
		<wp.blockEditor.InnerBlocks
			allowedBlocks={['custom/listitem']}
			templateInsertUpdatesSelection={false}
			renderAppender={appender}
		/>
		...
	);
};

@talldan
Copy link
Contributor

talldan commented Jun 14, 2019

Hey @websevendev,

I'm not entirely sure why IgnoreNestedEvents is not exposed. It might be that there are some considerations around its use, so the idea is not to promote it being used externally.

I think there might be two approaches to solve your problem:

  1. Expose IgnoreNestedEvents as you suggest. It would need some documentation.
  2. Add some improvements to InnerBlocks.ButtonBlockAppender so that you can pass in your own button text. You'd then be able to use that component instead of having to build your own button appender.

Is this an improvement that you might be willing to contribute? A good first step would be to make an issue, but a PR could also be an option. Let me know what you think.

@websevendev
Copy link

The benefits of <IgnoreNestedEvents> seem to be diminished now by #15537 since you can't directly click the button most of the time anyway, so it remains to be seen whether there's any reason to do anything.

Add some improvements to InnerBlocks.ButtonBlockAppender so that you can pass in your own button text. You'd then be able to use that component instead of having to build your own button appender.

My previous code example was a bit stripped down, it's not just a <button>, it's got an icon, matches the theme and can handle itself on a dark background, so for me just being able to control the text wouldn't be enough which is why renderAppender prop is great and InnerBlocks.ButtonBlockAppender should probably remain as simple as possible given that there's a powerful alternative.
image
There's possibilities to add multiple buttons, maybe duplicate last list item button, etc. So just exposing the IgnoreNestedEvents is probably the best or making it available with a prop

<wp.blockEditor.InnerBlocks
	renderAppender={appender}
	appenderIgnoreNestedEvents={true}
/>

or perhaps enabled by default and a prop to disable it? I wonder what the use cases would be for not using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P Needs Accessibility Feedback Need input from accessibility Needs Technical Feedback Needs testing from a developer perspective. [Status] In Progress Tracking issues with work in progress [Type] Feedback Issues that relate purely to feedback on a feature that isn't necessarily actionable
Projects
None yet
Development

Successfully merging this pull request may close these issues.