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: InnerBlock templates sync conditions to avoid a forced locking #9674

Merged
merged 2 commits into from Sep 20, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Sep 6, 2018

Description

Fixes: #8109

The InnerBlocks component executed block sync with the template each time the component mounted (or each time the component updated if the template changed).

During the document load, the InnerBlocks component is mounted again. So we had a bug even if a block used templateLock=false if blocks were added in an InnerBlocks area with a template during the load all the additional blocks were removed because of the sync.

How has this been tested?

I used the following test block https://gist.github.com/jorgefilipecosta/9ca39e388f6c46881809f0323f681197 and I checked that if I added new blocks e.g: a paragraph after saving and reloading the blocks are still present in the inner blocks area.
I verified that the columns block continues to work as before.

Screenshots

Before:
sep-06-2018 20-13-15-b

After:
sep-06-2018 20-13-43-a

@jorgefilipecosta jorgefilipecosta self-assigned this Sep 6, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Sep 7, 2018

@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Sep 14, 2018

@belcherj

This comment has been minimized.

Show comment
Hide comment
@belcherj

belcherj Sep 14, 2018

Contributor

Tested and this change works great for blocks with templates where users add more blocks after block is added. Before, those extra blocks would go missing.

Contributor

belcherj commented Sep 14, 2018

Tested and this change works great for blocks with templates where users add more blocks after block is added. Before, those extra blocks would go missing.

@haszari

This comment has been minimized.

Show comment
Hide comment
@haszari

haszari Sep 18, 2018

Is there any chance we can get this included in 3.9? We're using templates heavily, and the forced-sync issue blocks us from using Gutenberg in production. Currently, if anyone opens a Gutenberg-edited page in the editor, and re-saves, content will be lost due to the forced sync (when the content is loaded into the editor.

If there was any way to get this fix included in 3.9 we would be mightily appreciative!

haszari commented Sep 18, 2018

Is there any chance we can get this included in 3.9? We're using templates heavily, and the forced-sync issue blocks us from using Gutenberg in production. Currently, if anyone opens a Gutenberg-edited page in the editor, and re-saves, content will be lost due to the forced sync (when the content is loaded into the editor.

If there was any way to get this fix included in 3.9 we would be mightily appreciative!

@tofumatt

The code here seems fine but this could use an E2E test to make sure we don't hit a regression because this is probably not a common manual test case and I don't think it's a super-common use case for many users.

Feel free to ping me again if you want review on the E2E test, but please do add a test before merging 😄

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 20, 2018

Member

An end 2 end test that catches this bug was added.

Member

jorgefilipecosta commented Sep 20, 2018

An end 2 end test that catches this bug was added.

@jorgefilipecosta jorgefilipecosta merged commit e5d77bd into master Sep 20, 2018

2 checks passed

codecov/project 48.66% (-0.03%) compared to d5bbd81
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/innerblock-templates-forced-sync branch Sep 20, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 20, 2018

Member

Hi @haszari, unfortunately, I don't think we can get this in 3.9 as this is not a bug that was introduced in 3.9-RC. But this will get included in 4.0rc which should be released very soon, sorry for the inconvenience caused.

Member

jorgefilipecosta commented Sep 20, 2018

Hi @haszari, unfortunately, I don't think we can get this in 3.9 as this is not a bug that was introduced in 3.9-RC. But this will get included in 4.0rc which should be released very soon, sorry for the inconvenience caused.

@haszari

This comment has been minimized.

Show comment
Hide comment
@haszari

haszari commented Sep 20, 2018

Thank you @jorgefilipecosta !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment