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

State: Set template validity on block reset #9916

Merged
merged 1 commit into from Sep 17, 2018

Conversation

Projects
2 participants
@aduth
Member

aduth commented Sep 14, 2018

Extracted from #9403

This pull request seeks to refactor block validity to perform as a side-effect of the RESET_BLOCKS action (and temporarily, the SETUP_EDITOR action). This better reflects that testing block validity is not intended as an explicit action, but rather a function of comparing blocks state and editor settings. Until now I did not realize there was an intended user action around overriding validity ("Keep as is" on invalid template), so this is not intended to become deprecated.

Testing instructions:

Verify there are no regressions in the use of templates and template locking.

Here's a dummy CPT if you need one:

<?php

/**
 * Plugin Name: Demo CPT
 */

add_action( 'init', function() {
	register_post_type( 'book', [
		'label' => 'Book',
		'show_in_rest' => true,
		'public' => true,
		'show_ui' => true,
		'supports' => [ 'title', 'editor' ],
		'template' => [
			[ 'core/html', [
				'content' => '<p>Hello</p>',
			] ]
		],
		'template_lock' => 'all',
	] );
} );

@aduth aduth added the Templates API label Sep 14, 2018

@aduth aduth added this to the 4.0 milestone Sep 14, 2018

@aduth aduth added this to In Progress in API freeze via automation Sep 14, 2018

@aduth aduth requested review from youknowriad and jorgefilipecosta Sep 14, 2018

@jorgefilipecosta

I did some tests and I did not notice any regression when comparing against the behavior in master 👍
But I noticed an existing problem, as during each change on the code editor we call reset blocks, the "Keep it as is" option is not respected when using the code editor, even a character change in a valid block is enough to trigger the warning.
sep-14-2018 19-48-40

I left some comments but none of them are a blocker and feel free to discard some of them. I don't expect that checkTemplateValidity was being used by anyone besides us so I think it may be acceptable to just warn about its deprecation and remove its behaviour.

*
* @return {?Object} New validity set action if validity has changed.
*/
export function validateBlocksToTemplate( action, store ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 14, 2018

Member

As right now, besides the RESET_BLOCKS action this function also receives the SETUP_EDITOR_STATE, and my expectation is that this function will probably never need anything from the action besides the blocks, would it make sense for this function to receive the blocks instead of the action?

@jorgefilipecosta

jorgefilipecosta Sep 14, 2018

Member

As right now, besides the RESET_BLOCKS action this function also receives the SETUP_EDITOR_STATE, and my expectation is that this function will probably never need anything from the action besides the blocks, would it make sense for this function to receive the blocks instead of the action?

This comment has been minimized.

@aduth

aduth Sep 17, 2018

Member

So part of my purpose here in extracting this to a named function is to try to set a precedent for well-named / intentioned effect handlers. As you propose, we'd need a function which itself calls to validateBlocksToTemplate. It would likely start small, just extracting the blocks property from the action, but there'd be nothing to stop someone from adding to it, regressing to the poorer code standard.

Illustrated by code:

// Bad:

RANDOM_ACTION( action, store ) {
	// Do A

	// Do B

	// Do C
}

// Good:

RANDOM_ACTION: [
	doA( action, store ) {},
	doB( action, store ) {},
	doC( action, store ) {},
]

In general, I would have no issue with what you propose, and it'd probably make things even more clearly intentioned and easier to test. With needing the template and template lock details, it becomes a little more effort to be able to organize.

I might poke around at it to see if I can find some happy compromise, but otherwise I'm not strongly inclined to change it.

@aduth

aduth Sep 17, 2018

Member

So part of my purpose here in extracting this to a named function is to try to set a precedent for well-named / intentioned effect handlers. As you propose, we'd need a function which itself calls to validateBlocksToTemplate. It would likely start small, just extracting the blocks property from the action, but there'd be nothing to stop someone from adding to it, regressing to the poorer code standard.

Illustrated by code:

// Bad:

RANDOM_ACTION( action, store ) {
	// Do A

	// Do B

	// Do C
}

// Good:

RANDOM_ACTION: [
	doA( action, store ) {},
	doB( action, store ) {},
	doC( action, store ) {},
]

In general, I would have no issue with what you propose, and it'd probably make things even more clearly intentioned and easier to test. With needing the template and template lock details, it becomes a little more effort to be able to organize.

I might poke around at it to see if I can find some happy compromise, but otherwise I'm not strongly inclined to change it.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

Yes, that's a fair point, even a function that is just supposed to pass the blocks to another function can then be changed to a different purpose. Feel free to keep the current version as probably it will keep code more organized in the long run.

@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

Yes, that's a fair point, even a function that is just supposed to pass the blocks to another function can then be changed to a different purpose. Feel free to keep the current version as probably it will keep code more organized in the long run.

plugin: 'Gutenberg',
hint: 'Validity is verified automatically upon block reset.',
} );
return {
type: 'CHECK_TEMPLATE_VALIDITY',

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Sep 14, 2018

Member

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?
Imagining a plugin that adds blocks (wrongly without verifying the locking) after each block insert the plugin called checkTemplateValidity to have an explicit check and if necessary the warning would appear, now that will not work right?
I think the version should be 4.1 and not 4.0.

@jorgefilipecosta

jorgefilipecosta Sep 14, 2018

Member

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?
Imagining a plugin that adds blocks (wrongly without verifying the locking) after each block insert the plugin called checkTemplateValidity to have an explicit check and if necessary the warning would appear, now that will not work right?
I think the version should be 4.1 and not 4.0.

This comment has been minimized.

@aduth

aduth Sep 17, 2018

Member

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?

I suppose we should try to keep the behavior intact, yes. This probably explains why I hadn't removed it from #9403.

I think the version should be 4.1 and not 4.0.

Will change 👍

@aduth

aduth Sep 17, 2018

Member

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?

I suppose we should try to keep the behavior intact, yes. This probably explains why I hadn't removed it from #9403.

I think the version should be 4.1 and not 4.0.

Will change 👍

This comment has been minimized.

@aduth

aduth Sep 17, 2018

Member

In the rebased b94c076, I both restored the CHECK_TEMPLATE_VALIDITY effect handler and changed the deprecation version from 4.0 to 4.1.

@aduth

aduth Sep 17, 2018

Member

In the rebased b94c076, I both restored the CHECK_TEMPLATE_VALIDITY effect handler and changed the deprecation version from 4.0 to 4.1.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

But I noticed an existing problem, as during each change on the code editor we call reset blocks, the "Keep it as is" option is not respected when using the code editor, even a character change in a valid block is enough to trigger the warning.

Hmm, makes me wonder whether we should consider this to be the expected result. If a user modifies their post content in the code editor to a state such that it invalidates the template, shouldn't we want to present them with this warning?

Related questions:

  • We shouldn't be triggering the block reset on each character press, only on blur. It wasn't clear from your comment whether there's an issue here.
  • In master, we already call checkTemplateValidity() on the code editor's onPersist callback, so I expect this shouldn't be different at all?
Member

aduth commented Sep 17, 2018

But I noticed an existing problem, as during each change on the code editor we call reset blocks, the "Keep it as is" option is not respected when using the code editor, even a character change in a valid block is enough to trigger the warning.

Hmm, makes me wonder whether we should consider this to be the expected result. If a user modifies their post content in the code editor to a state such that it invalidates the template, shouldn't we want to present them with this warning?

Related questions:

  • We shouldn't be triggering the block reset on each character press, only on blur. It wasn't clear from your comment whether there's an issue here.
  • In master, we already call checkTemplateValidity() on the code editor's onPersist callback, so I expect this shouldn't be different at all?
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

I checked the behavior of the template validation on Code Editor changes and it behaves the same between this branch and master, which is to prompt about the invalid template when conflicting changes are made and the user proceeds to blur the field. I expect that this is and should be the expected behavior (evidenced further by the fact that we explicitly called the template validation check previously in this callback).

Member

aduth commented Sep 17, 2018

I checked the behavior of the template validation on Code Editor changes and it behaves the same between this branch and master, which is to prompt about the invalid template when conflicting changes are made and the user proceeds to blur the field. I expect that this is and should be the expected behavior (evidenced further by the fact that we explicitly called the template validation check previously in this callback).

@aduth aduth merged commit c28d2db into master Sep 17, 2018

2 checks passed

codecov/project 48.76% (+0.02%) compared to 0816e23
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Sep 17, 2018

@aduth aduth deleted the remove/template-validity-actions branch Sep 17, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 17, 2018

Member

Hi, @aduth that's right the behavior is the same as in master. I understand it may be expected behavior, but in my personal preference, I would prefer if the "keep it as is" was valid for all the editing session anyway I'm sorry for the off topic.
I checked that if I manually call wp.data.dispatch('core/editor').checkTemplateValidity(); the warning appears again (if the blocks don't match the template) and things continue to work as before.

Member

jorgefilipecosta commented Sep 17, 2018

Hi, @aduth that's right the behavior is the same as in master. I understand it may be expected behavior, but in my personal preference, I would prefer if the "keep it as is" was valid for all the editing session anyway I'm sorry for the off topic.
I checked that if I manually call wp.data.dispatch('core/editor').checkTemplateValidity(); the warning appears again (if the blocks don't match the template) and things continue to work as before.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

I would prefer if the "keep it as is" was valid for all the editing session anyway I'm sorry for the off topic.

It's good to raise I think. I don't really have a strong opinion on the matter. I could imagine changing the value of template.isValid to a tri-state null, false, or true, where only null -> false can occur, but not true -> false. Has a few repercussions, e.g. will never flag template invalidation in the same session if it was originally valid but made to become invalid.

Member

aduth commented Sep 17, 2018

I would prefer if the "keep it as is" was valid for all the editing session anyway I'm sorry for the off topic.

It's good to raise I think. I don't really have a strong opinion on the matter. I could imagine changing the value of template.isValid to a tri-state null, false, or true, where only null -> false can occur, but not true -> false. Has a few repercussions, e.g. will never flag template invalidation in the same session if it was originally valid but made to become invalid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment