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

@jorgefilipecosta jorgefilipecosta 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
Copy link
Member

gziolo commented Apr 11, 2019

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

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Apr 11, 2019
@jorgefilipecosta
Copy link
Member Author

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

@oandregal
Copy link
Member

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 Compare April 30, 2019 13:50
@oandregal
Copy link
Member

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 Compare June 4, 2019 08:20
@jorgefilipecosta
Copy link
Member Author

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.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants