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

Rename "Content only" template lock API to something else #45613

Closed
talldan opened this issue Nov 8, 2022 · 9 comments
Closed

Rename "Content only" template lock API to something else #45613

talldan opened this issue Nov 8, 2022 · 9 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Code Quality Issues or PRs that relate to code quality

Comments

@talldan
Copy link
Contributor

talldan commented Nov 8, 2022

The template lock feature currently has the following values:

  • all - everything is locked
  • insert - blocks cannot be inserted
  • false - there is no template locking

#43037 introduced a new 'contentOnly' template lock feature that allows content to be edited. The naming of this new feature is inconsistent. The existing values imply the aspect of the blocks that are locked, so 'contentOnly' would mean that content can't be edited.

If there's the possibility of renaming this it would be good to do it early. It's difficult to come up with suggestions. Maybe something like 'editableContent'. @noisysocks suggested 'chrome' or 'allButContent'.

cc @jorgefilipecosta

@talldan talldan added [Feature] Block API API that allows to express the block paradigm. [Type] Code Quality Issues or PRs that relate to code quality labels Nov 8, 2022
@stokesman
Copy link
Contributor

stokesman commented Nov 17, 2022

It seems there’s no getting around the fact that the new feature is a greater level of locking than the existing all and any name is going to conflict with the concept of all. So wouldn't the fix be to surface the new feature separately and have it override templateLock when enabled. Something like:
editOnlyContent: boolean
Or maybe:
editable = 'all': 'all' | 'content'

If we really hate having them separate the latter of those suggestions (or something like it) could expand to include the templateLock features so as to deprecate it (though it'd probably never become unsupported)

@noisysocks
Copy link
Member

noisysocks commented Nov 20, 2022

It seems there’s no getting around the fact that the new feature is a greater level of locking than the existing all

This isn't correct, is it? all locks everything but contentOnly allows the content to be edited. Hence my (bad) suggestion of renaming contentOnly to allButContent.

@stokesman
Copy link
Contributor

Screen recording demonstrating that contentOnly is a greater level of locking than all:

demo-template-lock.mp4

If anyone cares to try it for themselves here’s the code for the block demo'd in the recording:

A snippet that can be pasted into the console that registers a block with a template and configurable `templateLock`.
// Template lock testing block
( ( {
  blocks: { registerBlockType },
  blockEditor: {
    InspectorControls,
    useInnerBlocksProps,
  },
  components: { PanelBody, SelectControl },
  element: { createElement: el, Fragment },
} ) => {
  registerBlockType( 'test/template-lock', {
    title: 'Test Template Lock',
    edit: Edit,
    attributes: { templateLock: { default: 'all' }},
    save: () => null,
  } );
  function Edit({attributes: { templateLock }, clientId, setAttributes}) {
    return (
      el( Fragment, null,
        el( InspectorControls, null,
          el( PanelBody, {title: 'Template Lock'},
            el( SelectControl, {
              onChange: v => setAttributes({templateLock: v}),
              value: templateLock,
              options: [
                {value: 'all', label: 'All'},
                {value: 'insert', label: 'Insert'},
                {value: 'contentOnly', label: 'Content Only'},
                {value: false, label: 'None (false)'},
              ]
            }),
          )
        ),
        el( 'div', useInnerBlocksProps({}, {
          templateLock: templateLock === 'false' ? false : templateLock,
          template: [
            ['core/paragraph', {content:'How do I lock thee?'}],
            ['core/columns', {}, [
              ['core/column', {}, [
                ['core/paragraph', {content:'Let me count the ways.'}]
              ]],
              ['core/column', {}, [
                ['core/paragraph', {content:'“all”, “insert”, “contentOnly”, false'}]
              ]]
            ]],
            ['core/separator', { className:'is-style-wide'}],
            [
              'core/social-links', {
                layout:{type:'flex',justifyContent:'center'},
                size:'large',
              }, [
                ['core/social-link', {service:'wordpress', url: '❤️'}]
              ]],
          ]
        }))
      )
    );
  };
} )( wp );

From what I gather, templateLock was designed for the scope of inner block placement. That is, insertion, movement, or deletion and all pertains to just that. This new level of locking doesn't fit cleanly into that scope.

@noisysocks
Copy link
Member

Aha, thanks :)

Curious what @WordPress/gutenberg-core thinks. If we're going to clarify this API best to do so now before it's too late.

@talldan
Copy link
Contributor Author

talldan commented Nov 22, 2022

That's a great point about it being a stronger level of locking. That's nuance that's easily missed.

Around the idea of separate properties, a question is would you ever have templateLock set to false along with editable: 'content'? Being able to set templateLock to its own value probably contradicts the purpose of contentOnly, so I think that's a reasonable argument for making it part of templateLock.

@oandregal
Copy link
Member

For reference, the names considered/used at some point:

In essence, the thing I struggle the most with any of the names proposed so far is how it fits with the other values:

  • while insert and all communicate what they lock, the new name communicates what it does not lock
  • the new name is not composable or related to the existing values: it operates at a higher level than all and, at the same time, it allows the users to do more things than all (edit)

I don't think I have any suggestion, but wanted to share this context.

@talldan
Copy link
Contributor Author

talldan commented Nov 23, 2022

Thanks for sharing the history @oandregal.

it allows the users to do more things than all (edit)

I think you can still edit with 'all', so 'contentOnly' seems purely a higher level of locking.

Given this is now documented, it may not be the best idea to change it.

@mtias
Copy link
Member

mtias commented Nov 23, 2022

The naming problem is just a symptom of the fact this isn't really about template locking, or better put, it's combining a couple features that we should aim to untangle a bit:

  • Controlled children.
  • Template locking (disabling movers, appenders, removals).

Controlled children is about being able to select the parent and see the children in the sidebar, or selecting the children and still seeing the parent as a unit. This is a cool feature but is actually not dependant on template locking. For example, the new navigation block is making use of a very similar view without locking the contents.

You could combine controlled children + template locking to get this "patterns feel like a single block where you only modify content". The original work aimed at making it simple for a pattern author to make this combination with a single flag.

@gziolo gziolo added [Feature] Templates API Related to API powering block template functionality in the Site Editor Needs Technical Feedback Needs testing from a developer perspective. labels Feb 27, 2023
@talldan talldan removed the Needs Technical Feedback Needs testing from a developer perspective. label Apr 11, 2023
@talldan
Copy link
Contributor Author

talldan commented Feb 19, 2024

Closing as this is impossible to change now.

@talldan talldan closed this as not planned Won't fix, can't repro, duplicate, stale Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

6 participants