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

Fix: Impossible to drag & drop blocks if locking is insert #14521

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Copy link
Member

commented Mar 19, 2019

Description

If the CPT locking is set to insert it should be possible to move the blocks. Currently, it is possible to move the blocks using the block move buttons but not possible using drag & drop.

This PR fixes the problem of being impossible to use drag & drop when locking equals insert.

How has this been tested?

I pasted the contents of this gist https://gist.github.com/jorgefilipecosta/b50b090932b22bff1abe193fd0f5d649 in the functions.php file of the theme enable on my test website. I verified I can move the blocks using drag & drop while on master that is not possible.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/impossible-to-drag-drop-blocks-if-locking-is-insert branch from fe5dc81 to 6d3e49b Apr 12, 2019

@nosolosw nosolosw self-requested a review Apr 22, 2019

@nosolosw
Copy link
Member

left a comment

This indeed renders the dropzone, but when dropping the block in another place it doesn't work - blocks stay in the same position.

I've also checked that changing the template_lock to all in the example CPT provided disabled the block mover.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Hi @nosolosw thank you for review this PR. It depends on #14924 if both are merged things should work as expected.

@nosolosw

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Cherry picked this commit on top of #14924 but still didn't work.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/impossible-to-drag-drop-blocks-if-locking-is-insert branch from 6d3e49b to d59b0bb Apr 30, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the fix/impossible-to-drag-drop-blocks-if-locking-is-insert branch from d59b0bb to 98c1ebe Apr 30, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Hi @nosolosw, it seems my analysis was wrong. This PR is not dependent on #14924. The condition to make this move work was already part of the moveBlockToPosition function. The problem was that fromRootClientId was '' and toRootClientId was undefined so the condition fromRootClientId === toRootClientId did not matched. I updated the code to make things work as expected, and this PR should fix the problem on its own.

@nosolosw

This comment has been minimized.

Copy link
Member

commented May 2, 2019

OK, this is what I've found: testing with the example provided works as described.

I also wanted to test how this behaves with inner blocks, so I've added array( 'core/columns' ) to the provided template. These are the results:

with template_lock=insert:

  • top-level blocks:

    • I can move them.
    • I can move a block from the top-level to within the columns block. Is this expected? It was a bit weird to me given that I then can't move it outside (well, actually, I can by using the undo button).
  • columns' inner blocks:

    • I can only move them within the columns block. I guess this makes #14924 redundant? Note that, in this case, the blue lines for the drop zones outside the columns block are still shown.
    • The inserter is available and I can add new blocks. Do we want to address this in a different issue?

with template_lock=all:

  • top-level blocks: the movers are disabled and I can't move them.
  • columns' inner blocks:
    • The inserter is available and I can add new blocks.
    • The movers are not disabled.

I guess we can prepare a different PR for fixing the template_lock=all issues.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

I also wanted to test how this behaves with inner blocks, so I've added array( 'core/columns' ) to the provided template. These are the results:

Hi @nosolosw, this seems like the expected behavior given how locking inheritance works.

A block inherits the locking from the parent if locking was not explicitly set.
The columns block sets a locking all. The column block sets a locking false to remove all the locking restrictions.
So if we set locking insert or all for the CPT and we add columns in the template, inside each column there is no locking at all given that the block removes the locking. The columns need a locking because each column is managed by the block UI so the column block needs to remove the locking.

So I think given the inheritance logic we don't have any issue in the template_lock=all.

I can only move them within the columns block. I guess this makes #14924 redundant?

That restriction happens and the move is not possible because there is a locking insert on root level. #14924 addresses another issue where the move is allowed if we don't have locking on the root level, but we have locking insert in a child block.

This logic is complex, I hope I managed to summarize it, but If I missed something in my explanation feel free to comment and I will expand.

@nosolosw
Copy link
Member

left a comment

Works as expected and the code is fine.

I can repro in master the issues I've reported in #14521 (comment) so this PR doesn't change the current behavior. Thanks for the added explanation about the locking mechanism, Jorge. My expectations on how should it work were different, but any further decision on updating it is orthogonal to this PR.

@jorgefilipecosta jorgefilipecosta merged commit 1385adf into master May 7, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/impossible-to-drag-drop-blocks-if-locking-is-insert branch May 7, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.