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

Use controlled InnerBlocks in reusable blocks #23601

Closed

Conversation

chrisvanpatten
Copy link
Member

Description

A small PR branched from #21427 which switches reusable blocks from a nested BlockEditorProvider area to the controlled InnerBlocks API introduced in #21368.

How has this been tested?

Tested in browser.

Types of changes

Non-breaking change to reusable blocks.

@chrisvanpatten chrisvanpatten added [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Block] Block The "Reusable Block" Block labels Jul 1, 2020
@chrisvanpatten chrisvanpatten self-assigned this Jul 1, 2020
@github-actions
Copy link

github-actions bot commented Jul 1, 2020

Size Change: -52 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-library/index.js 129 kB -52 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.39 kB 0 B
build/block-directory/style-rtl.css 941 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 109 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.41 kB 0 B
build/block-library/editor.css 7.41 kB 0 B
build/block-library/style-rtl.css 8.04 kB 0 B
build/block-library/style.css 8.05 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.9 kB 0 B
build/compose/index.js 9.65 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/index.js 9.88 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.51 kB 0 B
build/edit-post/style.css 5.51 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.04 kB 0 B
build/edit-site/style.css 3.04 kB 0 B
build/edit-widgets/index.js 9.32 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.86 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.73 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.3 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@chrisvanpatten
Copy link
Member Author

chrisvanpatten commented Jul 1, 2020

The test failures seem legitimate but I'd love @ZebulanStanphill to validate that the fix actually solves the problem identified in #21427 before I clean those up.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

On the bright side, the reusable block no longer appears empty for a moment when switching between editing and not editing.

However, it looks like things are overall a lot more broken at the moment.

Try inserting two of the same reusable block, and then editing one of them. The other one will disappear until you stop editing.

Also, editing the reusable block is pretty broken. Try typing a bunch of characters in the reusable block and then saving; the saved version will be missing the last few characters. This lines up with the current end-to-end test failures.

Maybe some of this has to do with handleModifyBlocks no longer being called upon the input event? Not really sure.

@chrisvanpatten
Copy link
Member Author

Try inserting two of the same reusable block, and then editing one of them. The other one will disappear until you stop editing.

This one is super interesting. I wonder if @noahtallen has any ideas about this — perhaps there's something about the way controlled InnerBlocks handles state that would implicitly "link" the two instances together?

Also, editing the reusable block is pretty broken. Try typing a bunch of characters in the reusable block and then saving; the saved version will be missing the last few characters. This lines up with the current end-to-end test failures.

This is definitely concerning, and does seem likely related to onInsert vs onChange. It's possible onChange needs to be called more frequently, or perhaps that controlled InnerBlocks may need to support onInsert in some way.

@ZebulanStanphill
Copy link
Member

@chrisvanpatten I think controlled InnerBlocks already supports onInput, seeing as that the Template Part block uses it.

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jul 1, 2020

I just checked, and it turns out the can't-have-more-than-one kind of problem isn't exclusive to reusable blocks. A similar bug happens with Template Part (recently renamed to Section) blocks. See #22639.

@chrisvanpatten
Copy link
Member Author

@chrisvanpatten I think controlled InnerBlocks already supports onInput, seeing as that the Template Part block uses it.

Rad! I didn't know! Going to need to go back and update some of my own code…

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jul 1, 2020

Unfortunately, adding onInput handling doesn't seem to have changed anything.

@chrisvanpatten
Copy link
Member Author

@ZebulanStanphill I think it did help? In the first commit, "Reusable blocks › can be inserted and edited" and "Reusable blocks › can be converted to a regular block" failed, but in the onInput commit only "Reusable blocks › can be inserted and edited" failed.

The remaining issue definitely stems from #22639 though.

@ZebulanStanphill
Copy link
Member

@chrisvanpatten Huh. I tested out the branch myself earlier and the behavior didn't seem any different. But I just tested again and now it looks like the typing-quickly issue is fixed. I guess it might have been a browser cache issue on my end? Anyway, you're right; that issue is fixed. 😃

@noahtallen
Copy link
Member

I wonder if @noahtallen has any ideas about this

I bet the issue you linked to covers everything! Another observation is that there is a significant amount of state in block-editor/store which does extra stuff just to handle reusable blocks. (e.g. makes sure they are excluded from the root entity, etc) Those seemed kinda hacky to me when I was working with that state a while ago, given that it is overriding the default behavior just for one block type. I wonder if that state is compatible with the controlled inner blocks state.

@johnstonphilip
Copy link
Contributor

Just noting that this would also solve these additional issues:

  • You can add more than 1 block to a reusable block (maybe that's fine?)
  • You do not get access to the sidebar controls for any block inside a reusable block.

@ZebulanStanphill
Copy link
Member

  • You can add more than 1 block to a reusable block (maybe that's fine?)

Not entirely sure what you mean, but a reusable block containing multiple blocks at the top-level has always been allowed and intended. The reusable block itself acts like a parent block, albeit one that does not produce any wrapping markup on the front-end.

@johnstonphilip
Copy link
Contributor

@ZebulanStanphill Good to know! Then disregard that sentence :)

Base automatically changed from update/reusable-block-edit-hooks to master October 13, 2020 19:51
@ZebulanStanphill
Copy link
Member

This needs a rebase now that #21427 is merged. However, it looks like it may be blocked either way until #22639 is resolved.

@youknowriad
Copy link
Contributor

I missed this branch. I opened a new fresh one (similar in purpose here) #27887

@youknowriad youknowriad deleted the update/reusable-block-controlled-innerblocks branch December 24, 2020 14:36
@chrisvanpatten
Copy link
Member Author

Thanks for taking over here, @youknowriad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Block The "Reusable Block" Block [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants