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 move block to position bug; Add test cases; #14924

Merged
merged 2 commits into from Aug 5, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented Apr 11, 2019

Description

On #14003 I missed a case where a restriction was still not being applied and the moving blocks were possible when it should not be.

Basically, if an InnerBlocks are contained templateLock insert we should be able to move inside the area but not move an InnerBlock to another root block, that move was still possible.

I also add test cases to the moveBlockToPosition action.

How has this been tested?

Add the blocks available in https://gist.github.com/jorgefilipecosta/c9a9cc1c5199aca6786413492c302ccd.
Add the product block.
Verify none of its nested blocks can be moved outside. (before they could).

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@jorgefilipecosta - can you apply some labels? It looks like a bug, right?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Sorry, I should have applied the labels during the creation but the problem was corrected.

@nosolosw

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I've tested this and it works: I couldn't move the inner blocks outside the parent with the fix applied. One thing to note, though, is that the "blue line" indicating a DropZone is still shown.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/moveBlockToPosition-bug-add-test-cases branch 3 times, most recently from bd25877 to ad35b0e Apr 30, 2019

@nosolosw

This comment has been minimized.

Copy link
Member

commented May 2, 2019

For other reviewers: wanted to note that this may be made redundant as per feedback at #14521 (comment)

@jorgefilipecosta jorgefilipecosta force-pushed the fix/moveBlockToPosition-bug-add-test-cases branch from ad35b0e to 4ae5396 Jun 4, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

I've tested this and it works: I couldn't move the inner blocks outside the parent with the fix applied. One thing to note, though, is that the "blue line" indicating a DropZone is still shown.

Hi @nosolosw, the blue line appears in most cases even if the move ends up not being possible (e.g: allowed blocks restrictions, child blocks) so the fact that the blue line appears in this PR is a general problem.

I think we should address the general problem. I think we should expose a selector canMoveBlock... similar to canInsertBlock and in the drop zones components, we can use that selector. I think we can first merge this PR to avoid merge conflicts and then I can apply the refactor.

@draganescu
Copy link
Contributor

left a comment

I have tested this using the provided instructions and it works as described.

@jorgefilipecosta jorgefilipecosta merged commit 659b327 into master Aug 5, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/moveBlockToPosition-bug-add-test-cases branch Aug 5, 2019

@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 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.