-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Another paragraph is inserted, inside InnerBlocks. #10733
Fix: Another paragraph is inserted, inside InnerBlocks. #10733
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to wait until componentDidMount
to sync the template, or can it be done in componentWillMount
?
Alternatively, can we not render BlockList
until after the sync?
For what reason do we pass template
to BlockList
? The responsibilities are fuzzy to me, and makes me wonder if it's not BlockList
to be synchronizing the template. But as I look to BlockList
, it doesn't appear we should be passing it (it's unused)?
Honestly, this feels like something where we're being punished for our non-ideal implementation of InnerBlocks
template managing blocks state in response to lifecycle, rather than having more strict rendering control of blocks in a BlocksList
(i.e. value
or blocks
property, similar to some ideas @youknowriad has expressed around <Editor value={ ... } onChange={ ... } />
).
b748a47
to
dd6a5f9
Compare
Thank you for your review @aduth!
I think it can be done in componentWillMount, and it worked correctly in my tests if the only change is replacing componentDidMount with componentWillMount. I did not use this approach because componentWillMount will be deprecated.
I like the idea of not rendering BlockList until the sync is done, and it solves the problem of passing template sync info to BlockList. I updated the code to use this approach. Regarding the other template related props, you raised a very important question, I think these props are currently not being used (not certain why they were being passed probably they were needed before). I added a commit that removes them.
I agree that having the syntax of <Editor value={ ... } onChange={ ... } /> would be ideal, it is probably a task that we should analyze for the next phase, right now pursuing this approach seems risky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow the code here, and I think having an E2E test that reproduced the testing instructions here would help to contextualise the code/issue better and also prevent regressions.
Could you add an E2E test for this?
59912cf
to
7bbb60c
Compare
Squashed commits: [23554fe7b] Fix: Another paragraph is inserted, inside the block even if the block template already includes a paragraph. [a31f7b2ee] Update snapshot
7bbb60c
to
13bda1b
Compare
Hi @tofumatt thank you for reviewing this PR. We already had an end 2 end test case touching this, the snapshot had the buggy implementation and this PR updated the snapshot. But I agree we could benefit from a test dedicated to this behavior. I added a new end 2 end test specifically for this behavior. |
the end 2 end test was added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! 👍
Thank you for the reviews! |
Currently, we have a bug even if a block has a paragraph as the last block of the template if no locking exists another paragraph is inserted.
This happens because, before the template is processed the parent block is empty and DefaultBlockApppender is rendered right away. DefaultBlockApppender inserts blocks when it gets focused. The parent block has a mechanism to focus the first table and if InnerBlocks were not yet processed and the parent block does not contain inputs the DefaultBlockApppender is going to get the focus.
Before the template is processed we don't know what blocks will exist so during this phase DefaultBlockApppender should not be rendered. This PR makes sure that we don't render DefaultBlockApppender right after the block is inserted before the template is processed.
This behavior was noticed in #9416.
How has this been tested?
I added the test block available in https://gist.github.com/jorgefilipecosta/edafb2422ef41020d75619adf31d725e.
I checked that after inserting the first paragraph specified in the template gets focused and no other paragraph is created.
I checked the blocks depending on the behavior of DefaultBlockApppender inserting a block (columns) continue to work as before.
Screenshots
test block: https://gist.github.com/jorgefilipecosta/edafb2422ef41020d75619adf31d725e
After:
Before: