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

Additional lock options for template_lock option with block templates #5208

Closed
4 tasks
nerrad opened this issue Feb 23, 2018 · 15 comments
Closed
4 tasks

Additional lock options for template_lock option with block templates #5208

nerrad opened this issue Feb 23, 2018 · 15 comments
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Enhancement A suggestion for improvement.

Comments

@nerrad
Copy link
Contributor

nerrad commented Feb 23, 2018

Issue Overview

The current state of the template feature (see #3588) has two options for locking the template which are controlled via the template_lock option with the template registration:

  • all: which locks the position of all blocks and prevents any insertion of new blocks.
  • insert: which allows the blocks in the template to be moved around but no insertion of new blocks.

This issue is a request/proposal that additional iterations of the template feature include the following changes.

1. Change lock state to be accumulative instead of switch.

Instead of all or insert. Allow the lock types to be accumulatively applied and describe what specifically is locked. So you'd have something like this for the options:

  • insert: restricts any new blocks from being added
  • removal: restricts blocks in the template from being removed.
  • move: restricts blocks in the template from being moved.

So if someone wanted to have the template default to the current blocks but still allow them to be moved and removed but not allow the insertion of new blocks they could do something like this:

array(
...
'template_lock' => array('insert')
....
)

If we wanted to prevent insertion of new blocks and the removal of existing blocks but still allow existing blocks to be moved around we'd do:

array(
    ...
    'template_lock' => array('insert', 'removal')
    ...
)

2. Allow block specific locking.

With this, we'd allow specific blocks to be locked. In this case they'd only have two template_lock options: removal, or move. This way authors of templates could choose to restrict only a subset of blocks in a template. If they had the blocks restricted from moving, then that would just mean that in relation to all other blocks in the template that are also locked from moving, the locked block would keep its position.

Why?

One use case I've been thinking of for a plugin is where a template defines the following:

  • a block must be in the template for the plugin functionality in that content (eg. an event post type that has a ticket selector). This block can be moved to any location in the template but cannot be removed.
  • a block that the template defaults with that is optional (eg. a venue linked to the event that can be positioned anywhere in the content or removed if the author doesn't want to display venue information for this event).
  • a block for extra event data that is not displayed in the content but is required data for the event, we want this block's position locked so it can't be moved or removed. Sidebar doesn't work for the ui/ux of this content because of the size of the sidebar. (eg. picking what template is used for notifications related to this event when people register for it).

Related tickets

Todos

  • Discuss viability (yes? no? punt to future?)
  • Implementation - js/php
  • Tests
  • Documentation
@youknowriad
Copy link
Contributor

Does it make sense to propose a "remove" level and an "insert" level knowing that if you insert blocks, it makes sense to me to be able to remove them and the opposite is true?

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

I'm sorry I'm not sure I'm understanding your question and maybe your question is due to a misunderstanding of the proposal in the issue. Currently the documentation has this as the options for template_lock:

all — prevents all operations.
insert — prevents inserting new blocks, but allows moving existing ones.

My interpretation of the documentation is that either currently template_lock option prevents removal of blocks or the insertion of new blocks, but the insert level simply changes things so that existing blocks in the template can be moved around.

The problem here is the language of the options (assuming my interpretation of the documentation is correct). In part, what I'm proposing has two levels:

At the global template level:

template_lock option what it does
insert restricts new blocks that are not a part of the template from being added (without this option in the template_lock setting, new blocks not a part of the template can be added)
move restricts blocks that are a part of the template from being moved.
remove restricts blocks that are a part of the template from being removed.

At the template block level (blocks that are a part of the template only):

template_lock option what it does
move restricts blocks that are a part of the template from being moved.
remove restricts blocks that are a part of the template from being removed.

I think your question is under the assumption that move and remove would apply to new blocks added by the user in the editor for a templated view. I only see them applying to the blocks loaded as a part of the template.

Does that help clarify things?

@youknowriad
Copy link
Contributor

You're right, I misunderstood the issue originally. My bad.

Right now, there's no way to say that a block was part of the template or not, and my personal opinion at the moment is that adding this complexity is not worth-it. Technically, this is way too complex to implement, it requires constant matching between templates and blocks (and this would always be error prone) because the template do not identify the blocks and saving a post don't identify which blocks come from the template or not.

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

Right now, there's no way to say that a block was part of the template or not,

The template blocks are registered right now as a part of the template feature? So technically, that is known.

Are you opposing the entire proposal or just the per block template lock settings? It appears you're mainly opposed to the per block block template settings (but haven't really addressed the first item which is "1. Change lock state to be accumulative instead of switch." which is a change at the global template_lock setting level). With this first part of the proposal, I think we at least need a way to have a template defined default blocks that are included, but still provide some flexibility in how those blocks behave at the template level (can/cannot move, can/cannot remove, can insert new blocks that aren't a part of the template). Otherwise the template loses some of its flexibility.

As far as per block template settings go, let's look at removal controls for instance I see that the removeBlock control is being composed via the editor context and thus is only receiving the isLocked property from the editor component settings (thus global template settings exposed there). I think I grasp the complexity you are alluding to when it comes to exposing a specific block's "lock" settings. I suppose it wouldn't matter where the block settings are defined (either at the block level in the js, or via some map included with the template blocks array on load) right?

I realize there's complexity involved but I do think there are going to be cases where plugin/theme authors will want/need this functionality. At a minimum, whether its via template settings or individual block registration props, blocks need to be able to have some control over whether they can be moved or removed once they are added to editor by a template.

Let me describe a scenario (which I just alluded to in the proposal) that might help highlight why I created this issue. I'm a developer on the team that does the Event Espresso plugin and currently our plugin uses the classic editor interface for event authors to create/edit their events. A significant part of this interface is the creation of dates and times for the events along with tickets/prices etc. via a metabox. The data model is that datetimes are saved to their own table and related to events and tickets are saved to their own tables and related to datetimes. The data model requires that every event has at least one datetime and one ticket.

One way I'm considering we could integrate with gutenberg is the ticket editor could be a block. We'd have a default template for the event post type that inserts a useOnce ticket editor block and have that block locked so it can't be removed. In unselected state, the preview will show the ticket selector for all the configured tickets and they could move the block so it appears in the rest of the event post content wherever they want (functioning like a dynamicBlock in behaviour). The "selected" state for the block would expose the editor for configuring the tickets and datetimes and prices for the event.

Where this breaks down is:

  • there currently is no functionality/api exposed for being able to lock this block from being removed. If the event author removes this block then immediately that creates problems because the event does not have tickets or dates attached to it (but that could be mitigated somewhat by just changing our frontend and other processing so that event is just a description only.
  • IF we had the ability to lock the "ticket editor" block so its not removable, we'd still want to leave the ability for event authors to move that block around in the content (thus controlling the layout of where the ticket selector would appear in their content) and also still allow them the ability to insert new blocks into the editor (images etc).

Anyways, in part, this proposal is because I do see potential use-cases for it, but its also part of a discovery process in determining what approach we'll be able to take with integrating EE into Gutenberg. Obviously if the gutenberg team decides this proposal would make things too complicated and fragile and do not agree that there's sufficient use-case for the functionality, then we'll have to consider some other approach to do what we need in order to integrate.

@youknowriad
Copy link
Contributor

The template blocks are registered right now as a part of the template feature? So technically, that is known.

No, it's not because of the fact that you can insert/move blocks you don't know what blocks are part of the template or not unless you try to synchronize the template with the blocks constantly.

Are you opposing the entire proposal or just the per block template lock settings?

I'm not opposing the per block template lock settings but I'm opposing the way it's proposed here, I think all the usecases for this settings can be overcome using the nested lock settings we're exploring. You can lock everything and if you want an area to be "unlocked", you use a nested container block with an unlocked template.

"1. Change lock state to be accumulative instead of switch."

I'm not restricting this point neither I'm saying it's already accumulative in a way:

  • insert forbids inserting and removing blocks (because as explained, I think these are tied together)

  • move It doesn't make sense to allow inserting/removing blocks without allowing moving blocks, because you can just remove and insert at another position.


I'm not denying the use-case, I think it's legit, but I think it can be overcome using nested blocks locking (which is yet to come) without adding this granularity which creates complexity.

And I'm not entirely against adding a per block lock for some features like "this block can't be removed" but I prefer to wait until we get the nested locking working and see its concrete limits.

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

So just reviewing a bit of the code:

I see here that the <BlockRemoveButton> component receives a list of uids for the blocks. Would it be feasible that as blocks are created from a template, the uid of the block is kept in the store along with any block level "lock" settings. Then the store can be matched against the uids received on the BlockRemoveButton.props and if there's a remove lock then that BlockRemoveButton returns null for render? So something like this (pseudo code):

export function BlockRemoveButton( { onRemove, onClick = noop, isLocked, small = false, uids = [] } ) {
    const isBlockLocked = () => {
        if (isLocked) {
            return true;
        }
        return uids.some(function(uid){
            //new method exposing whether the block instance exists in the store containing references to template settings
            //for blocks created on load.
            if (templateHasBlock(uid)) {
                //new method retrieving the settings object for the block in the template block settings store.
                { blockLock } = getTemplateBlockSettings(uid);
                if (blockLock === 'remove') {
                    return true;
                }
            }
            return false;
        });
    };

    if ( isBlockLocked() ) {
        return null;
    }

    const label = __( 'Remove' );

    return (
        <IconButton
            className="editor-block-settings-menu__control"
            onClick={ flow( onRemove, onClick ) }
            icon="trash"
            label={ small ? label : undefined }
        >
            { ! small && label }
        </IconButton>
    );
}

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

sorry I was working on the code example before I saw your reply :)

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

And actually, in looking at the existing selectors. I see there already is a getBlock selector that can be fed a uid and spits out block info if a block with that uid exists in the store. Since the state of the block Attributes is exposed via this. I wonder if this could be used for a special blockLock attribute that could be used at the block level for controlling its individual lock restrictions (and how that applies to move/remove controls)? I realize this also means that the block could be unlocked programmatically via other means but I still think this would be a useful way for individual blocks to register their individual lock state and pass that to related controls?

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

By the way, I realize the team has an identified list of things they are working on. I'm willing to give a go at implementing this if there's openness to the idea. But of course I don't want to spend the time if its a definite no-go for this approach :)

@youknowriad
Copy link
Contributor

But of course I don't want to spend the time if its a definite no-go for this approach :)

It's definitely not a no-go from me, I just like iterative approaches :) especially when introducing complex features like this. Also, opinions are my own and might be good to have other opinions here @mtias @aduth

We iterated like this:

1- global templates
2- global locking
3- nested templates
4- next step: nested locking

At this point it would be good to make sure that these features are not sufficient to address the different use cases.

Potential future steps could include what this issue suggests, per block locking.

@youknowriad
Copy link
Contributor

Also, you may want to check this PR #5162 to understand what I mean by the complexity of matching templates and blocks.

@nerrad
Copy link
Contributor Author

nerrad commented Feb 23, 2018

Oh I see what you are referring now when you are talking about "matching templates and blocks". You are referencing how template definitions might change between when a post was first created with a template (and saved) and when its loaded after the shape of the template has changed. This also applies to if someone registers a template for a post type that includes plugin blocks but that plugin is no longer active when the post content is loaded.

So ya, with that consideration, I think we may want to keep global template state pretty much as is (but nested blocks could provide the ability for unlocking the "editor" instance in a nested block so new blocks can be added). However, I think having per block locks is still relatively manageable outside of the templates.

If I get some time this weekend I may work on experimenting with per block locks as described above and see how it goes.

@jeffpaul jeffpaul added the [Type] Enhancement A suggestion for improvement. label Mar 8, 2018
@mtias mtias added the [Feature] Templates API Related to API powering block template functionality in the Site Editor label Jul 17, 2018
@nerrad
Copy link
Contributor Author

nerrad commented Apr 29, 2019

I think this will prove to be very complex to implement and I'd prefer diving into this when there is a demonstrable use-case for implementing.

@nerrad nerrad closed this as completed Apr 29, 2019
@Glish
Copy link

Glish commented Aug 19, 2019

I have what I think is a use-case for these options, unless someone has an alternative use:

I have a single post type to generate user profile pages. In each page I have some custom meta fields ( photo, name, job_title, introduction). I have added these to my custom post type via functions.php and I have added a template to add my "intro" block to the post page by default.

My intro block has variables that are set to use the new meta fields via source: "meta".

This all works well, except for the fact that the admin can simply remove the block. Using 'template_lock' allows me to stop users removing it, but then they are unable to add any new blocks.

@senadir
Copy link
Contributor

senadir commented Sep 10, 2021

This is now partially addressed by #32457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

6 participants