Implement nesting in cover image block #5452

jorgefilipecosta · 2018-03-06T22:47:17Z

Description

This PR implements the concepts described in #5448.
E.g:

<InnerBlocks
	template={ [
		[ 'core/paragraph', {
			align: 'center',
			fontSize: 37,
			placeholder: 'Write title…',
			textColor: '#fff',
		} ],
	] }
	allowedBlockNames={ [ 'core/button', 'core/heading', 'core/paragraph', 'core/subhead' ] }
/>

As future work template_lock will be added. This opens the path for a whole new type of blocks.
Blocks that make use of nesting and just apply a special arrangement/design to existing blocks.
E.g: we can have a package of landing page blocks, where all of the blocks are based on paragraphs, headings, buttons etc... arranging them in a special way and applying some styles. Themes may take advantage of this to create theme-specific blocks.

The fact that the InnerBlocks settings are just props has some advantages e.g: depending on the settings in the block inspector we may allow different blocks. But it is much more complex to implement when compared to Add a property to the block API, allowedRootBlockNames: [ ... ] .

Right now this implementation is not very clean ( being nice to myself ) we are using a property in the state the save the current InnerBlock settings of each block uid with custom settings. This was required because we need to globally know the allowed blocks, so the inserter can filter them. Maybe there is a better way to solve this problem. I'm thinking about an impossible way to improve this and I'm totally open to suggestions.

To define templates we are using the same logic as the one used in CPT's but this makes the data structure strange in javascript (until know templates were only defined in PHP so this was unnoticeable).

Cover image block was updated to make use of this concepts.

Know problems / related work

Using the autocomplete, we can insert allowed blocks. Autocomplete logic is different from other inserters and we already have some problems e.g: if the template lock the blocks, autocomplete inserter is not aware of the lock and then we get a big js error because onReplace does not exist in this case. In parallel, I will improve the logic of autocomplete inserter so changes required here are easier.

Testing

Test the cover image block try to add blocks remove see things work as expected. The design will benefit from #5658.
Test that templates also work with layouts by adding this template to columns block.

                    template={ [
                        [ 'core/paragraph', {
                            placeholder: 'Enter column 1 content…',
                            layout: 'column-1',
                        } ],
                        [ 'core/paragraph', {
                            placeholder: 'Enter column 2 content…',
                            layout: 'column-2',
                        } ],
                    ] }

Test that we are backcompatible with existing cover images by pasting the following code in the code editor and verifying it works as expected:

<!-- wp:cover-image {"url":"http://via.placeholder.com/350x150","id":10559} -->
<div class="wp-block-cover-image has-background-dim" style="background-image:url(http://via.placeholder.com/350x150)">
	<p class="wp-block-cover-image-text">Test</p>
</div>
<!-- /wp:cover-image -->

Screenshots (jpeg or gifs if applicable):

aduth · 2018-03-07T02:54:42Z

Related: #4097 (as far as prior art in restricting block types shown in the inserter)

phpbits · 2018-03-15T02:45:33Z

@jorgefilipecosta looking forward to this :) Thanks!

youknowriad

Nice work here 👍

youknowriad · 2018-03-23T08:57:26Z

blocks/library/cover-image/editor.scss

+	.editor-block-list__layout {
+		width: 100%;
+		.editor-block-list__block:not(.is-multi-selected) .wp-block-paragraph {
+			background: none;


Do we use a background on the paragraph block by default? I'm trying to understand why this is needed mainly to avoid the specificity to target the paragraph block's className in another block.

Hi @youknowriad,
The reason why we set a white background is related to the contrast verification. If the background was never set getComputedStyles returns back background (if I remember correctly). So adding a background fixes the problem. If themes change the background it also works well because as long as a background is set we are able to retrieve it. The problem happens when no background is set. As we get back background color when background is not set we cannot differentiate and assume is white background because in fact it may be set to black. I will try to research this in more detail maybe there is a way to detect when the background is not set and in that case assume white.
Replicate from comment #5273 (comment).

A refactoring will happen to contrast checker in order to not rely on this rule. So it will be removed for now.

youknowriad · 2018-03-23T08:59:06Z

blocks/library/cover-image/index.js

+								textColor: '#fff',
+							} ],
+						] }
+						allowedBlockNames={ [ 'core/button', 'core/heading', 'core/paragraph', 'core/subhead' ] }


❤️ awesome API

Should we move it to the settings object in registerBlockType to make it extensible? :)

I meant both template and allowedBlockNames.

As noted in the description, this allows it to be customizable using toolbar/inspector controls.

But this makes me think of something. We should consider the template prop here as a defaultTemplate because we already have support for nested template. by setting a CPT template with a nested template for the cover image for instance. I think we should do something here where the template prop is used (override it by the global template if available or something)

I’m missing something here, how can I override those values for Cover Image block? 😄
I couldn't digest this from this PR.

Okey, I know what you were referring to. You should be able to do it when providing the template for CPT as explained in here: https://wordpress.org/gutenberg/handbook/templates/#nested-templates.

Can we switch this API to be allowedBlocks?

Hi @youknowriad,

But this makes me think of something. We should consider the template prop here as a defaultTemplate because we already have support for nested template. by setting a CPT template with a nested template for the cover image for instance. I think we should do something here where the template prop is used (override it by the global template if available or something).

When we have a CPT template and we set content for a nested block, ( e.g: we set a cover image with a header and a paragraph), during the post-load the cover image is created and the blocks are added inside it. So when our inner logic executes the cover image block already has content, so the template will not be used. It will be the same as when we load a post with a cover image that already has blocks inside it. The template only applies to empty blocks.
So both approaches can work together without problems if a global template exists it already overwrites this setting :)

Hi @mtias the prop was renamed to allowedBlocks.

youknowriad · 2018-03-23T09:02:47Z

editor/store/reducer.js

@@ -976,13 +976,41 @@ export const reusableBlocks = combineReducers( {
 	},
 } );

+export const blockListSettings = ( state = {}, action ) => {


Can you add a docBlock to explain what's this reducer for?

youknowriad · 2018-03-23T09:05:07Z

editor/components/block-list/block.js

+	insertTemplateBlocksIfInnerBlocksEmpty( template ) {
+		const { block, onInsertBlocks, uid } = this.props;
+		if ( template && ! block.innerBlocks.length ) {
+			onInsertBlocks( synchronizeBlocksWithTemplate( [], template ), 0, uid );


I think synchronizing the template is not needed here because we only synchronize templates if the template is locked but we don't support locking yet for inner Blocks.

Unless I'm missing something?

Yes synchronizing the template is not needed. But I want to given a template parse and create the block structure it represents turns out that's exactly what synchronizeBlocksWithTemplate( [], template ) does so I did not want to duplicate code and used it :)

youknowriad · 2018-03-23T09:08:06Z

editor/utils/block-list.js

+				}
+
+				componentWillReceiveProps( nextProps ) {
+					updateNestedSettingsIfNeeded( {


As far As I understand we have the uid at this point, so we can call the action directly in this component to avoid the need to add a new callback to this HoC?

youknowriad · 2018-03-23T09:09:22Z

editor/utils/block-list.js

+					updateNestedSettingsIfNeeded( {
+						supportedBlocks: this.props.allowedBlockNames,
+					} );
+					insertTemplateBlockIfNeeded( this.props.template );


Same for this callback: can w call the action directly in this component to avoid the need to add a new callback to this HoC?

youknowriad · 2018-03-23T09:11:27Z

editor/components/inserter-with-shortcuts/index.js

+		( state, { globalEnabledBlockTypes } ) => {
+			const insertionPoint = getBlockInsertionPoint( state );
+			const { rootUID } = insertionPoint;
+			const blockListSettings = getBlockListSettings( state, rootUID );


Maybe we could move this logic to a selector to test it properly:

getSupportBlocks( state, uid, enabledBlockTypes )

Second that 👍

youknowriad · 2018-03-23T09:12:55Z

editor/components/inserter/index.js

@@ -79,19 +80,25 @@ class Inserter extends Component {
 						onClose();
 					};

-					return <InserterMenu onSelect={ onSelect } />;
+					return <InserterMenu onSelect={ onSelect } supportedBlockTypes={ supportedBlockTypes } />;


It looks like this prop is not used in the current component, can we move to the menu component instead?

Also might be good to factorize, using the selector proposed above?

Hi @youknowriad, the prop supportedBlockTypes is used to compute hasSupportedBlocks flag that's why it was computed in this component.
Regarding the factorize to selector, I will try to make this more generic maybe it will involve selector plus HoC that accesses the context.

gziolo · 2018-03-23T10:00:49Z

To define templates we are using the same logic as the one used in CPT's but this makes the data structure strange in javascript (until know templates were only defined in PHP so this was unnoticeable).

It doesn't look that bad, it is similar to what is used in config files for libraries like Babel. See: https://github.com/WordPress/packages/blob/master/packages/babel-preset-default/index.js#L6-L11

gziolo · 2018-03-23T10:05:20Z

blocks/library/cover-image/index.js

 	url: {
 		type: 'string',
 	},
-	align: {


I think align should stay in here. Otherwise, it won't be possible to change the alignment of the container.

Yes, the idea I have in mind is that it is no possible to change the alignment of the container. The container takes 100% width. So for example, if we want to right align text we apply the alignment on the nested paragraph block.

Edited: sorry I understood what you mean, you are totally right it was a mistake. This attribute should not be removed.

gziolo · 2018-03-23T10:07:29Z

blocks/library/cover-image/index.js

 		const controls = isSelected && [
 			<BlockControls key="controls">
-				<BlockAlignmentToolbar


The same applies in here, unless it is moved to the InnerBlock. In that case validAlignments and getEditWrapperProps might need to be removed.

gziolo · 2018-03-23T10:13:22Z

blocks/library/cover-image/index.js

+								textColor: '#fff',
+							} ],
+						] }
+						allowedBlockNames={ [ 'core/button', 'core/heading', 'core/paragraph', 'core/subhead' ] }


I’m missing something here, how can I override those values for Cover Image block? 😄
I couldn't digest this from this PR.

gziolo

I left a few comments to check.

gziolo · 2018-03-28T04:49:52Z

blocks/library/cover-image/index.js

+							[ 'core/paragraph', {
+								align: 'center',
+								fontSize: 37,
+								placeholder: 'Write title…',


Translation wrapper missing for placeholder.

gziolo · 2018-03-28T04:52:08Z

blocks/library/cover-image/index.js

+						template={ [
+							[ 'core/paragraph', {
+								align: 'center',
+								fontSize: 37,


Font size has now this t-shirt like sizes, is it one of them?

gziolo · 2018-03-28T04:58:56Z

editor/utils/block-list.js

-				componentWillMount() {
-					INNER_BLOCK_LIST_CACHE[ uid ][ 1 ]++;
-				}
+			connect( ( state ) => {


It might be a good practice to stop using connect in favor of withSelect snd withDispatch HOCs.

gziolo · 2018-03-28T05:03:25Z

editor/components/inserter-with-shortcuts/index.js

+		( state, { globalEnabledBlockTypes } ) => {
+			const insertionPoint = getBlockInsertionPoint( state );
+			const { rootUID } = insertionPoint;
+			const blockListSettings = getBlockListSettings( state, rootUID );


Second that 👍

gziolo · 2018-03-28T05:07:09Z

blocks/test/fixtures/core__cover-image.html

-<!-- wp:core/cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg","dimRatio":40} -->
-<div class="wp-block-cover-image has-background-dim-40 has-background-dim" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg)">
-    <p class="wp-block-cover-image-text">Guten Berg!</p>
+<!-- wp:cover-image {"url":"https://cldup.com/uuUqE_dXzy.jpg","id":8398} -->


has-undefined-content is here because contenAlign attribute was removed, but not its corresponding class name.

gziolo · 2018-03-28T05:09:41Z

blocks/test/fixtures/core__cover-image.json

            "hasParallax": false,
-            "dimRatio": 40
+            "dimRatio": 50


It might be nice to keep non-default dim to see the class name included in HTML.

gziolo · 2018-03-28T05:11:27Z

blocks/test/fixtures/core__cover-image.html

+<div class="wp-block-cover-image has-background-dim has-undefined-content" style="background-image:url(https://cldup.com/uuUqE_dXzy.jpg)">
+    <div class="wp-block-cover-image__inner-container">
+        <!-- wp:paragraph {"align":"center","placeholder":"Write title…","textColor":"#fff","fontSize":37} -->
+        <p style="color:#fff;text-align:center">Paragraph 1</p>


Is it expected that fot size is not printed as style?

jorgefilipecosta · 2018-03-28T22:11:56Z

Hi @gziolo and @youknowriad thank you a lot for the feedback it was applied.
These changes need additional test cases which I will add in parallel. But the logic is ready for a new look.
Regarding extensibility, and allowing plugins to change the inner block settings, unfortunately, this PR does not address it (in order to not increase the size), but my plan is to creates a new filter that allows plugins to change all the props passed to InnerBlocks (allowedBlocks, template, etc...), the filter receives an object with the settings and can return a new one that changes them. There are other ways like a more granular allowedBlocks filter. after we land the core changes (or agree that they are the path to go) I will add a PR that allows plugins to change these settings :)

jasmussen · 2018-04-25T13:28:08Z

Wanted to note that I'm doing some work in #6406 that might benefit some of the color/UI issues.

jorgefilipecosta · 2018-04-26T09:01:24Z

Hi @mcsf, the rebase was done. I think this should ready. I'm solving some accessibility issues using other PR's (as they are not exclusive in this PR).
Regarding the design, many improvements are already present and @jasmussen is making things even better in a side PR. I was just waiting for a green light from the UX&UI perspective.

jorgefilipecosta · 2018-04-26T09:18:25Z

Hi @jasmussen, thank you for the improvements you are making they will make nesting in colored places like cover image better.
I personally think it would be ok to merge this even before your PR as I think the UI is already in an acceptable state for most backgrounds and merging would allow testing the UI changes in real use cases even easily, and could potentially allow some further works that take advantage of the mechanisms implemented here.

karmatosed

I think whilst it's not perfect visually getting this in and refining with the work @jasmussen is doing there, is the best route. As a result approving this and let's note though in merge that this is to be refined, just to set expectations.

jasmussen · 2018-04-26T09:44:11Z

Since this is a pretty complex PR, should we wait for Matías technical thoughts? He's AFK but hopefully back next week. It feels like his comments starting in #5452 (comment) were sort of skeptical. Not saying because this isn't impressive work, just wondering.

Adds a set of Gutenberg "Form" blocks for use with the contact form module. Blocks: * Form - The main block, consists of an InnerBlock for the <form> element and a set template consisting of a default set of inner blocks preconfigured as a contact form * Text - General <input type=text>/<textarea> field with configurable label, rows * Button - A <button type=submit> with configurable button text Blocking Release: * InnerBlock templating requires WordPress/gutenberg#5452. * WordPress/gutenberg#5452 needs to override isPrivate settings to prevent form blocks being seen in the inserter when out of form context. * WordPress/gutenberg#5452 is whitelist only meaning we cant add extra blocks in between form elements - not really a huge issue for this but likely an issue for other use cases. Todo - Integration with grunion-contact-form: * Captcha support should be relatively easy to achieve with a serverside rendered block * A server side filter / hook could replace the form action Todo - Handle duplicate field names gracefully * Currently uses a variation on the label / field name to set id / for settings on label / inputs. * Currently uses same variation for <input name=""> attributes

gziolo · 2018-05-10T08:49:46Z

@jorgefilipecosta - can you rebase with the recent changes from master?

@mtias - it waits for your feedback.

ZebulanStanphill · 2018-06-22T02:56:52Z

Should this be added to the Merge Proposal: Editor milestone? It seems like something that would be good for optimum recreation of existing content into Gutenberg block form.

@oskosk

Context: #9452 @oskosk Requested this unfinished branch to be pushed up to Automattic/jetpack for use in the Jetpck Beta Tester Plugin. -- Adds a set of Gutenberg "Form" blocks for use with the contact form module. Blocks: * Form - The main block, consists of an InnerBlock for the <form> element and a set template consisting of a default set of inner blocks preconfigured as a contact form * Text - General <input type=text>/<textarea> field with configurable label, rows * Button - A <button type=submit> with configurable button text Blocking Release: * Fix issues identified in original PR #9452 * InnerBlock templating requires WordPress/gutenberg#5452. * WordPress/gutenberg#5452 needs to override isPrivate settings to prevent form blocks being seen in the inserter when out of form context. * WordPress/gutenberg#5452 is whitelist only meaning we cant add extra blocks in between form elements - not really a huge issue for this but likely an issue for other use cases. Todo - Integration with grunion-contact-form: * Captcha support should be relatively easy to achieve with a serverside rendered block * A server side filter / hook could replace the form action Todo - Handle duplicate field names gracefully * Currently uses a variation on the label / field name to set id / for settings on label / inputs. * Currently uses same variation for <input name=""> attributes

ZebulanStanphill · 2018-08-06T15:55:29Z

I think the Cover Image block would be made redundant by the existence of a Container block. The Cover Image concept could just be a Reusable block inserted as a template. Think about it. Every setting for the Cover Image block would work for a Container block, and adding nesting to the Cover Image block just makes it into a sort of container block anyway.

Implement nesting in cover image block #5452

Implement nesting in cover image block #5452

Conversation

jorgefilipecosta commented Mar 6, 2018 • edited Loading

Description

Know problems / related work

Testing

Screenshots (jpeg or gifs if applicable):

aduth commented Mar 7, 2018

phpbits commented Mar 15, 2018

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo commented Mar 23, 2018

Choose a reason for hiding this comment

jorgefilipecosta Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Mar 28, 2018

jasmussen commented Apr 25, 2018

jorgefilipecosta commented Apr 26, 2018

jorgefilipecosta commented Apr 26, 2018 • edited Loading

karmatosed left a comment

Choose a reason for hiding this comment

jasmussen commented Apr 26, 2018

gziolo commented May 10, 2018

ZebulanStanphill commented Jun 22, 2018 • edited Loading

ZebulanStanphill commented Aug 6, 2018

jorgefilipecosta commented Mar 6, 2018 •

edited

Loading

jorgefilipecosta Mar 28, 2018 •

edited

Loading

jorgefilipecosta Mar 27, 2018 •

edited

Loading

jorgefilipecosta Mar 27, 2018 •

edited

Loading

jorgefilipecosta Mar 27, 2018 •

edited

Loading

gziolo Mar 28, 2018 •

edited

Loading

jorgefilipecosta commented Apr 26, 2018 •

edited

Loading

ZebulanStanphill commented Jun 22, 2018 •

edited

Loading