Block Templates: Check the validity of the block list against the defined template #5162

youknowriad · 2018-02-20T12:08:38Z

When editing a post, the template is ignored entirely which means that if you update the template and opens an existing post in the editor, the template won't be updated.

This PR adds a mechanism to check the content of the current post against the template. The check is considered valid if all the block names of the content match the block names of the template.

If the check is invalid a notice is displayed inviting the user to reset its post according to the template (which could restult in some content loss) or keep the content as is.

If you edit the post in the text editor, the check is also performed.

Note This PR is necessary in order to achieve nested template locking because we want to match which container block has which lock config in the template.

Testing instructions

You can make use of the GCF plugin to test this quickly.

Add a locked template for your post
Create and save a post
Edit the post in the texteditor and remove some blocks
When clicking outside of the text editor, the notice is shown
When you refresh the page the notice is shown.
Try both buttons in the notice.

aduth · 2018-02-23T04:55:18Z

editor/components/template-notice/index.js

+
+	return (
+		<Notice isDismissible={ false } status="warning">
+			<div className="editor-template-notice">


Could we update Notice to apply an optional className prop?

aduth · 2018-02-23T04:56:50Z

editor/store/actions.js

 *
 * @return {Object} Action object.
 */
-export function setupEditorState( post, blocks, edits ) {
+export function setupEditorState( post, blocks, edits, isValidTemplate ) {


I'm not sure this needs to be part of setupEditorState, particularly because it's not part of state.editor like the others are (and notably the others needing to be bulked together for undo/dirtying reasons).

aduth · 2018-02-23T04:57:57Z

editor/components/post-text-editor/index.js

 		},
 		onPersist( content ) {
-			return resetBlocks( parse( content ) );


We use redux-multi, so this could have been updated to return as an array.

aduth · 2018-02-23T04:59:14Z

blocks/api/templates.js

+ * @return {boolean}        Whether the list of blocks matches a templates
+ */
+export function doesBlocksMatchTemplate( blocks = [], template = [] ) {
+	return blocks.length === template.length && every( template, ( [ name,, innerBlocksTemplate ], index ) => {


Aversion to long lines makes me wish this was:

if ( blocks.length !== template.length ) { return false; } return every( /* ... */ );

These double commas can be easy to overlook. Personally I add a blank space between them, though not really sure it's any better. Thoughts?

I added a space, I agree it's not common so it should be more visible.

aduth · 2018-02-23T05:02:09Z

blocks/api/templates.js

+ *
+ * @return {Array}          Updated Block list.
+ */
+export function synchronizeBlocksWithTemplate( blocks = [], template = [] ) {


The function name "sounds" mutative. Not sure I have a better alternative.. maybe getSynchronizedTemplate ?

I think the blocks should be in the name somehow, because it doesn't return a template, it returns blocks.

mtias · 2018-02-24T17:15:37Z

blocks/api/templates.js

+ *
+ * @return {boolean}        Whether the list of blocks matches a templates
+ */
+export function doesBlocksMatchTemplate( blocks = [], template = [] ) {


This should be doBlocksMatch...

mtias · 2018-02-24T17:15:44Z

blocks/api/templates.js

+ * @return {Array}          Updated Block list.
+ */
+export function synchronizeBlocksWithTemplate( blocks = [], template = [] ) {
+	return map( template, ( [ name, attributes, innerBlocksTemplate ], index ) => {


It'd be good to explain what happens to blocks (content) when they are synced.

mtias · 2018-02-24T17:15:56Z

editor/components/template-notice/index.js

@@ -0,0 +1,50 @@
+/**


maybe template-validation-notice.

mtias · 2018-02-24T17:31:32Z

editor/store/effects.js

 		if ( post.content.raw ) {
 			blocks = parse( post.content.raw );
+			isValidTemplate = (


Wonder if we should mention here we only validate templates that are defined as "locked".

jasmussen · 2018-02-27T10:59:25Z

Note This PR is necessary in order to achieve nested template locking because we want to match which container block has which lock config in the template.

Just to understand, this means when you create a template where adding or removing blocks is not allowed, correct?

Looking at this:

What happens if you press "Keep as is" and reload the page? Do you get the banner back? What if you accidentally pressed "Keep as is" and then regret it?

What happens if you press "Reset Template"?

Not asking as critique, just want to understand fully before I can help with design feedback. Thanks.

youknowriad · 2018-02-27T11:33:30Z

What happens if you press "Keep as is" and reload the page? Do you get the banner back? What if you accidentally pressed "Keep as is" and then regret it?

Yes, you get it back.

What happens if you press "Reset Template"?

Blocks that match the template are kept the onces that don't are removed. I expect this feature (locked templates) to be used with blocks saving to post meta which means in general, you won't lose anything, maybe you just added a new meta block and it will show up or you moved your meta blocks around.

jasmussen · 2018-02-27T11:44:00Z

Blocks that match the template are kept the onces that don't are removed.

Could you could step back to an older revision if you lost some content?

youknowriad · 2018-02-27T11:51:55Z

Could you could step back to an older revision if you lost some content?

You can undo if it's not saved, and if it's saved, there's always revisions :)

mtias · 2018-02-27T13:27:15Z

to be used with blocks saving to post meta

Not sure if this'll be the case. I can see many locked templates that just want to define an initial shape. Might be worth considering to support migrations for individual blocks within a template.

youknowriad · 2018-03-06T17:25:51Z

Not sure how to move forward here? Is there anything missing? Do we disagree with the idea entirely?

mtias · 2018-03-06T17:42:07Z

Sorry, let's get it in as it applies to locked templates alone.

…ined template

youknowriad added the [Feature] Templates API Related to API powering block template functionality in the Site Editor label Feb 20, 2018

youknowriad self-assigned this Feb 20, 2018

youknowriad requested review from mtias, aduth and a team February 20, 2018 12:08

mtias added the Needs Design Feedback Needs general design feedback. label Feb 21, 2018

youknowriad mentioned this pull request Feb 22, 2018

GCF template do not appear in existing posts youknowriad/gcf#26

Closed

aduth reviewed Feb 23, 2018

View reviewed changes

youknowriad force-pushed the add/template-validity-check branch from d760bd2 to 2f8cc52 Compare February 23, 2018 09:24

youknowriad mentioned this pull request Feb 23, 2018

Additional lock options for template_lock option with block templates #5208

Closed

4 tasks

mtias reviewed Feb 24, 2018

View reviewed changes

editor/components/template-notice/index.js Outdated

@@ -0,0 +1,50 @@

/**

Copy link

Member

mtias Feb 24, 2018

Choose a reason for hiding this comment

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

maybe template-validation-notice.

mtias reviewed Feb 24, 2018

View reviewed changes

youknowriad added 7 commits March 19, 2018 14:42

Block Templates: Check the validity of the block list against the def…

eac82ea

…ined template

Components: Support Notice custom className

9a804ac

Extract the isValidTemplate from the setupEditorState action

c2f36e7

Rely on redux-multi to dispatch multiple actions

673ff1a

Avoid long lines

1906526

Fix Typo in Function name doBlocksMatchTemplate

ca3ef26

Template validation: adding some comments

c7fda73

youknowriad force-pushed the add/template-validity-check branch from 26488c5 to bbfb64f Compare March 19, 2018 13:43

Rename the TemplateValidationNotice component

68eaeca

youknowriad force-pushed the add/template-validity-check branch from bbfb64f to 68eaeca Compare March 19, 2018 13:47

youknowriad merged commit d64de0f into master Mar 19, 2018

youknowriad deleted the add/template-validity-check branch March 19, 2018 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Templates: Check the validity of the block list against the defined template #5162

Block Templates: Check the validity of the block list against the defined template #5162

youknowriad commented Feb 20, 2018 •

edited

Loading

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

mtias Feb 24, 2018

mtias Feb 24, 2018

mtias Feb 24, 2018

mtias Feb 24, 2018

jasmussen commented Feb 27, 2018

youknowriad commented Feb 27, 2018

jasmussen commented Feb 27, 2018

youknowriad commented Feb 27, 2018

mtias commented Feb 27, 2018

youknowriad commented Mar 6, 2018

mtias commented Mar 6, 2018

Block Templates: Check the validity of the block list against the defined template #5162

Block Templates: Check the validity of the block list against the defined template #5162

Conversation

youknowriad commented Feb 20, 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

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

jasmussen commented Feb 27, 2018

youknowriad commented Feb 27, 2018

jasmussen commented Feb 27, 2018

youknowriad commented Feb 27, 2018

mtias commented Feb 27, 2018

youknowriad commented Mar 6, 2018

mtias commented Mar 6, 2018

youknowriad commented Feb 20, 2018 •

edited

Loading