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

inherit templateLock in column block #23329

Closed
wants to merge 3 commits into from

Conversation

oxyc
Copy link
Member

@oxyc oxyc commented Jun 19, 2020

Description

Second take on #17301 since #20465 was merged. As described in #17301 and #17262, when having template_lock = 'all' on a post template, the column blocks are not locked and allow any block to be inserted.

Now that the columns block doesn't explicitly set a templateLock the fix to inherit the locking settings seems to be as easy as removing the override in the Column block.

How has this been tested?

  1. Added a template to the post post type.
add_action('init', function () {
    $post_type_object = get_post_type_object('post');
    $post_type_object->template = [
      [ 'core/paragraph', [
        'placeholder' => 'test'
      ] ],
      [ 'core/columns', [ 'align' => 'wide' ], [
        [ 'core/column', [], [
          [ 'core/paragraph', [ 'placeholder' => 'col 1' ] ]
        ] ],
        [ 'core/column', [], [
          [ 'core/paragraph', [ 'placeholder' => 'col 2' ] ]
        ] ],
      ] ]
    ];
    $post_type_object->template_lock = 'all';
});
  1. Create a new post and notice how the template is present and you cannot add new blocks within the column block.
  2. Manually edit HTML content of a column, adding another paragraph and notice that The content of your post doesn’t match the template assigned to your post type notice allowing you to reset the template.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@oxyc
Copy link
Member Author

oxyc commented Jun 19, 2020

ping @jorgefilipecosta since you provided feedback on my first PR (edit: just noticed you're on leave though :) )

@oxyc oxyc requested review from nerrad and ntwb as code owners June 20, 2020 01:16
@oxyc
Copy link
Member Author

oxyc commented Jun 20, 2020

So apparently the e2e tests are reflecting the opposite behavior (#22115). At least @jorgefilipecosta showed some interest in this behavior earlier #17301 (review).

What do others think? I havent updated the text fixtures since I'm currently having issues running the e2e tests locally, but if you can agree this is the correct behavior I'll look into getting the fixtures updated again.

@oxyc
Copy link
Member Author

oxyc commented Jun 20, 2020

My use case for having it locked is that I can lock site editors to a predefined template. Currently columns are an exception to this. All other blocks become locked from the post template (groups, media-text, covers etc).

@oxyc oxyc force-pushed the core-column-templatelock-2 branch from c10bbbb to 2f0757b Compare June 20, 2020 14:23
@oxyc oxyc added [Block] Columns Affects the Columns Block [Feature] Templates API Related to API powering block template functionality in the Site Editor [Package] Block library /packages/block-library labels Jun 20, 2020
@oxyc oxyc force-pushed the core-column-templatelock-2 branch from 9c58476 to ba9a774 Compare July 2, 2020 00:51
@jorgefilipecosta
Copy link
Member

Hi @oxyc thank you for proposing a new simpler solution to this issue. It seems like it would make the column inside columns inherit the locking from parent blocks (or the CPT) this would make column consistent with what happens with the group.
@youknowriad, @mtias any thoughts on this, do you think we can apply this change?

@youknowriad
Copy link
Contributor

So the question to answer is whether we think applying a "lock" at the global level should inherit or not in the column block.
It it's very hard to answer, for me I do wonder whether template locking inheritance is a good thing at all. When I have a template, I can also decide to leave a group/columns blocks empty for people to fill it and it seems like a valid use-case as well.

So what I wonder is whether instead of having locking inheritance built-in into InnerBlocks, we could remove the inheritance and add a templateLock attribute like the placeholder attribute (potentially even a template attribute) to some container blocks.

@jorgefilipecosta
Copy link
Member

So what I wonder is whether instead of having locking inheritance built-in into InnerBlocks, we could remove the inheritance and add a templateLock attribute like the placeholder attribute (potentially even a template attribute) to some container blocks.

Yes, a templateLock attribute would allow full control in parent blocks.

At the time, we did not follow this path because we were avoiding the addition of attributes that don't change anything at all on the block output, that's why we also discussed things like block options similar to attributes that just affect the edit of a block but not the output. None of the solutions discussed gained traction, and meanwhile, we used the placeholder attribute in more blocks, so I guess a template lock attribute would be a possible solution.

@oxyc
Copy link
Member Author

oxyc commented Aug 10, 2020

I can also decide to leave a group/columns blocks empty for people to fill it and it seems like a valid use-case as well.

Currently you can only do this for columns blocks though, not group. Which is a bit inconsistent.

Worth seeing the original issue #17262 where expected behavior was the opposite from my own expected behavior.

So what I wonder is whether instead of having locking inheritance built-in into InnerBlocks, we could remove the inheritance and add a templateLock attribute like the placeholder attribute (potentially even a template attribute) to some container blocks.

This would serve everyone much better for sure.

@jorgefilipecosta
Copy link
Member

This would serve everyone much better for sure.

In that case, I guess we can go for the attribute approach @youknowriad suggested and totally remove the inheritance mechanism?
This would need a dev note but I guess it is the right time to do a change like that as if we do it now we will have the whole 5.6 development cycle to receive feedback on the change and make sure it does not create a critical issue.

@mtias
Copy link
Member

mtias commented Aug 25, 2020

Also related: #8112. It'd also be good to consider use cases in patterns regarding locking. Also, if locking becomes an attribute, it should probably be exposed in the "Advanced" settings as a toggle, so that you can build patterns with parts of it locked. The presence of such UI should probably support permissions / capabilities. It might also get tricky with direct HTML editing, but that is already the case with any locking system — the attribute might actually help reconcile or discard changes since it leaves a trail in history.

@jorgefilipecosta
Copy link
Member

Hi @oxyc, @mtias, @youknowriad,

It seems the steps to close this issue are:

  • Remove locking inheritance (needs a dev note)
  • Add a locking attribute to the blocks. This attribute can be controlled from the advance panel on the block inspector.
  • Blocks can easily include the panel, and the attribute using an option supports flag.

And I can propose an implementation following this logic.

Users would be able to remove the locking set by CPT template, patterns, or parent blocks, using the advance panel. Is this something we accept?

@youknowriad
Copy link
Contributor

Add a locking attribute to the blocks. This attribute can be controlled from the advance panel on the block inspector.

I'd imagined that initially this attribute won't have any UI, more like the current "placeholder" attribute. Something developpers can use in their templates to control the locking behavior.

@tellthemachines
Copy link
Contributor

Given that #26128 has been merged, can this PR be closed now?

@youknowriad
Copy link
Contributor

yes, let's do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Feature] Templates API Related to API powering block template functionality in the Site Editor [Package] Block library /packages/block-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants