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

WIP: Blocks: Reimplement shared block as embedded editor #7453

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@aduth
Member

aduth commented Jun 21, 2018

Related: #7409, #7130
Closes #7119

Note: Work-In-Progress. Most basic behaviors work as expected, though lack some of the UX polish which had existed previously around intermediate states (saving, etc), unit tests, documentation.

This pull request seeks to explore a reimplementation of the shared block to render its editable form as a nested instance of <EditorProvider><BlockList /></EditorProvider>.

In doing so, it explores:

  • Inheriting context in EditorProvider to allow pass-through of slot fill handling
    • Shared block inspector controls surface to the main editor sidebar
  • Unique editor stores by core/data namespace (e.g. core/editor-shared-8174)
    • Workaround to the fact that all stores registered to @wordpress/data are shared in a common global namespace
  • Umbrella override of select calls within an element tree via withCustomReducerKey to avoid each descendent needing to be updated to account for the custom store namespace
  • Using existing Editor effects to leverage updates to the shared block (both its title and content), rather than a separate set of duplicated effects (saving a shared block uses the same REQUEST_POST_UPDATE since it is in-fact a post, rather than a custom SAVE_SHARED_BLOCK action)
    • In doing so, it removes the custom block endpoint so as to be able to treat the shared blocks as no different than any other post
  • Eliminating shared blocks as part of the top-level editor's own blocks state
    • Important for proper solution to what was initially explored as part of #7130 for autosave and dirty detection
  • Shared block components no longer need to pull data from the top-level data store tracking of state.sharedBlocks, since as being part of its own provider context, it can simply call the same selectors for edited post (e.g. getEditedPostAttribute( 'title' ) to retrieve the shared block title)

There's some future work here that may enable removing all shared block knowledge from the top-level editor, not addressed here:

  • Fully treating shared blocks as simply post entities to be fetched from the core data namespace
    • Fetching, deleting, creating
@noisysocks

Very clever! I haven't looked at this closely yet but I like the idea.

Just dropping a note to be careful when dropping the shared blocks API controller. It has a few workarounds in it which ensure that only certain roles can read and delete shared blocks, see #4725.

Show outdated Hide outdated phpunit/class-rest-blocks-controller-test.php Outdated
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jun 25, 2018

Member

Just dropping a note to be careful when dropping the shared blocks API controller. It has a few workarounds in it which ensure that only certain roles can read and delete shared blocks, see #4725.

Yeah, fair to say its removal is tentative, though I'd like to see it gone, merely for the inconsistencies it has with the posts responses, and the fact that it doesn't seem like it should be any more special than a standard post. Shouldn't the posts endpoint already be accounting for these capabilities?

Member

aduth commented Jun 25, 2018

Just dropping a note to be careful when dropping the shared blocks API controller. It has a few workarounds in it which ensure that only certain roles can read and delete shared blocks, see #4725.

Yeah, fair to say its removal is tentative, though I'd like to see it gone, merely for the inconsistencies it has with the posts responses, and the fact that it doesn't seem like it should be any more special than a standard post. Shouldn't the posts endpoint already be accounting for these capabilities?

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jun 26, 2018

Member

Shouldn't the posts endpoint already be accounting for these capabilities?

In theory, yes. I vaguely remember running into some issues with the WP API posts controller. So long as we confirm that the capability checking hasn't regressed (e.g. by keeping test_capabilities) then I'm happy 🙂

Member

noisysocks commented Jun 26, 2018

Shouldn't the posts endpoint already be accounting for these capabilities?

In theory, yes. I vaguely remember running into some issues with the WP API posts controller. So long as we confirm that the capability checking hasn't regressed (e.g. by keeping test_capabilities) then I'm happy 🙂

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 12, 2018

Member

Reference to original implementation as reference in preparation for pending rebase: 58c5103

Member

aduth commented Jul 12, 2018

Reference to original implementation as reference in preparation for pending rebase: 58c5103

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 12, 2018

Member

Rebased to account for the new data registry implemented by @youknowriad in #7527 .

This is still very much work-in-progress, but it's back to a workable state, and simpler too now with the new registry.

I'm curious to get feedback on the idea with cloning registries / creating editor stores. I added a clone function to the store object. This also requires tracking original "register" values, because we assign into the registry internal namespaces object using overridden behavior (e.g. selectors with pre-bound state).

Member

aduth commented Jul 12, 2018

Rebased to account for the new data registry implemented by @youknowriad in #7527 .

This is still very much work-in-progress, but it's back to a workable state, and simpler too now with the new registry.

I'm curious to get feedback on the idea with cloning registries / creating editor stores. I added a clone function to the store object. This also requires tracking original "register" values, because we assign into the registry internal namespaces object using overridden behavior (e.g. selectors with pre-bound state).

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jul 16, 2018

Member

I really like the overall approach!

I'm fine with how clone() is implemented especially since namespaces and _cloneConfig isn't accessible by callers of createRegistry(). This means we can iterate on how clone() works without breaking backwards compatibility. For example, in the future, it might be worth exploring using prototype inheritance to save some memory.

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

From my very quick testing, I spot these problems:

  • Progress indicator no longer shows after clicking Save
  • Inserter isn't updated when shared block title is changed
  • Exception when selecting Convert to Regular Block
  • Menu shows Convert to Shared Block instead of Convert to Regular Block after refreshing the page
  • Shared blocks aren't shown in the inserter after refreshing page
  • More Options and Move up / Move down shouldn't be shown within a shared block
  • Deleting a shared block does not remove the wp:block from the post
  • Needs tests and JSDoc comments
  • Need to confirm (e.g. using unit tests) that #4725 hasn't regressed

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

Member

noisysocks commented Jul 16, 2018

I really like the overall approach!

I'm fine with how clone() is implemented especially since namespaces and _cloneConfig isn't accessible by callers of createRegistry(). This means we can iterate on how clone() works without breaking backwards compatibility. For example, in the future, it might be worth exploring using prototype inheritance to save some memory.

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

From my very quick testing, I spot these problems:

  • Progress indicator no longer shows after clicking Save
  • Inserter isn't updated when shared block title is changed
  • Exception when selecting Convert to Regular Block
  • Menu shows Convert to Shared Block instead of Convert to Regular Block after refreshing the page
  • Shared blocks aren't shown in the inserter after refreshing page
  • More Options and Move up / Move down shouldn't be shown within a shared block
  • Deleting a shared block does not remove the wp:block from the post
  • Needs tests and JSDoc comments
  • Need to confirm (e.g. using unit tests) that #4725 hasn't regressed

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 16, 2018

Member

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

Yes! Please feel free.

Member

aduth commented Jul 16, 2018

Are you OK for me to pick this PR up and sand off the remaining rough edges? I'm not sure how your schedule looks 🙂

Yes! Please feel free.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jul 16, 2018

Member

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

❤️ ❤️ ❤️

Member

aduth commented Jul 16, 2018

The E2E tests that I added in 5eb5fdd should act nicely as a set of acceptance tests for this PR.

❤️ ❤️ ❤️

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jul 17, 2018

Member

I rebased and fixed some of the regressions identified above. Will continue on this tomorrow.

One regression that I'm struggling with is that changes to a shared block aren't picked up by other blocks on the page. For example:

  1. Insert the same shared block twice
  2. Make some changes to the first wp:block (e.g. change its title)
  3. Note that the second wp:block isn't updated with these changes

(The same issue manifests itself in a few different ways. For example, if you change a shared block's title, the inserter doesn't immediately update to have the new title.)

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

Does that make sense, @aduth? Do you have any ideas on how to address this?

Member

noisysocks commented Jul 17, 2018

I rebased and fixed some of the regressions identified above. Will continue on this tomorrow.

One regression that I'm struggling with is that changes to a shared block aren't picked up by other blocks on the page. For example:

  1. Insert the same shared block twice
  2. Make some changes to the first wp:block (e.g. change its title)
  3. Note that the second wp:block isn't updated with these changes

(The same issue manifests itself in a few different ways. For example, if you change a shared block's title, the inserter doesn't immediately update to have the new title.)

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

Does that make sense, @aduth? Do you have any ideas on how to address this?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 17, 2018

Contributor

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

While editing the shared blocks, I'd say it's acceptable, but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

Contributor

youknowriad commented Jul 17, 2018

I suspect that this happens because changes to a wp_block entity record are isolated to the registry that the change occurred in. That is, when editPost() is dispatched from one shared block editor, other editors aren't notified because they are subscribed to a different Redux store. Having seperate registries mean that we no longer have a single source of truth for wp_block entity records.

While editing the shared blocks, I'd say it's acceptable, but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jul 18, 2018

Member

but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

Unfortunately this doesn't work when previewing e.g. a shared columns block because <InnerBlocks> assumes that we have the nested blocks available in editor state.

Member

noisysocks commented Jul 18, 2018

but we should probably rely on the serialized value of the original store when we're not in edit mode. What I'm trying to say is that the separate registry should only be used when we click "edit" to edit the content of the shared block and not wrap the entire BlockEdit of shared blocks with a RegistryProvider.

Unfortunately this doesn't work when previewing e.g. a shared columns block because <InnerBlocks> assumes that we have the nested blocks available in editor state.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jul 18, 2018

Contributor

Unfortunately this doesn't work when previewing e.g. a shared columns block because assumes that we have the nested blocks available in editor state.

I guess I assumed that there's a two-way sync between both states:

<EmbedRegistry>
<EditorProvider onChange={updateSharedBlock} post={sharedBlock}>
</EditorProvider>
</EmbedRegistry>

Having this kind of API would be ace, not only for this shared block issue but also to reuse Gutenberg as a standalone editor (passing a value, and having changes persisted with onChange calls like any React component)

Contributor

youknowriad commented Jul 18, 2018

Unfortunately this doesn't work when previewing e.g. a shared columns block because assumes that we have the nested blocks available in editor state.

I guess I assumed that there's a two-way sync between both states:

<EmbedRegistry>
<EditorProvider onChange={updateSharedBlock} post={sharedBlock}>
</EditorProvider>
</EmbedRegistry>

Having this kind of API would be ace, not only for this shared block issue but also to reuse Gutenberg as a standalone editor (passing a value, and having changes persisted with onChange calls like any React component)

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Aug 21, 2018

Member

I should have a little bit of time this week to work on this. I've rebased the PR but am yet to test whether or not I've broken everything in the process.

Tasks remaining:

  • Check that I didn't completely botch the rebase 🙂
  • Fix regression described above in #7453 (comment)
  • Ensure issues identified above in #7453 (comment) are fixed:
    • Progress indicator no longer shows after clicking Save
    • Inserter isn't updated when shared block title is changed
    • Exception when selecting Convert to Regular Block
    • Menu shows Convert to Shared Block instead of Convert to Regular Block after refreshing the page
    • Shared blocks aren't shown in the inserter after refreshing page
    • More Options and Move up / Move down shouldn't be shown within a shared block
    • Deleting a shared block does not remove the wp:block from the post
    • Need to confirm (e.g. using unit tests) that #4725 hasn't regressed
  • Fix other regressions I'm just now noticing:
    • After converting a block to reusable, the title field should be selected
    • When two reusable blocks are next to each other, their margins overlap
    • "Block created." and "Block deleted." notices have weird padding
    • Delete Reusable Block button is missing after refreshing the page
  • Fix failing unit tests
  • Add back tests that were removed from gutenberg/packages/editor/src/store/test/effects.js
  • Ensure reusable block E2E tests pass
  • Add JSDoc comments to all new methods
Member

noisysocks commented Aug 21, 2018

I should have a little bit of time this week to work on this. I've rebased the PR but am yet to test whether or not I've broken everything in the process.

Tasks remaining:

  • Check that I didn't completely botch the rebase 🙂
  • Fix regression described above in #7453 (comment)
  • Ensure issues identified above in #7453 (comment) are fixed:
    • Progress indicator no longer shows after clicking Save
    • Inserter isn't updated when shared block title is changed
    • Exception when selecting Convert to Regular Block
    • Menu shows Convert to Shared Block instead of Convert to Regular Block after refreshing the page
    • Shared blocks aren't shown in the inserter after refreshing page
    • More Options and Move up / Move down shouldn't be shown within a shared block
    • Deleting a shared block does not remove the wp:block from the post
    • Need to confirm (e.g. using unit tests) that #4725 hasn't regressed
  • Fix other regressions I'm just now noticing:
    • After converting a block to reusable, the title field should be selected
    • When two reusable blocks are next to each other, their margins overlap
    • "Block created." and "Block deleted." notices have weird padding
    • Delete Reusable Block button is missing after refreshing the page
  • Fix failing unit tests
  • Add back tests that were removed from gutenberg/packages/editor/src/store/test/effects.js
  • Ensure reusable block E2E tests pass
  • Add JSDoc comments to all new methods

aduth and others added some commits Jul 12, 2018

Blocks: Reimplement shared block as embedded editor
- Publish shared blocks when creating and editing them
- Fix exception when converting a shared block to static
- Fix 'Convert to Regular Block' button
- Remove the block when deleting a shared block
Sync changes made to a reusable block
Update the editor's store when the reusable block is changed. This
ensures other instances of the same reusable block are updated.
<EditorProvider
// Force a rerender when reusableBlock is updated, so that changes made
// externally appear in the nested editor.
key={ reusableBlockInstanceId }

This comment has been minimized.

@noisysocks

noisysocks Aug 23, 2018

Member

Not very proud of this. Ideally, EditorProvider would update when its post prop changes but the SETUP_EDITOR effect makes this difficult...

@noisysocks

noisysocks Aug 23, 2018

Member

Not very proud of this. Ideally, EditorProvider would update when its post prop changes but the SETUP_EDITOR effect makes this difficult...

// Update the editor's store when the reusable block is changed. This
// ensures other instances of the same reusable block are updated.
receiveReusableBlocks( [ { reusableBlock, parsedBlock } ] );
receiveEntityRecords( 'postType', 'wp_block', reusableBlock );

This comment has been minimized.

@noisysocks

noisysocks Aug 23, 2018

Member

We should be able to remove the receiveReusableBlocks dispatch here once we make Gutenberg treat reusable blocks like any other entity.

@noisysocks

noisysocks Aug 23, 2018

Member

We should be able to remove the receiveReusableBlocks dispatch here once we make Gutenberg treat reusable blocks like any other entity.

@slimmilkduds

This comment has been minimized.

Show comment
Hide comment
@slimmilkduds

slimmilkduds Oct 3, 2018

Any updates on this, could this be expected to get merged before 5.0?

slimmilkduds commented Oct 3, 2018

Any updates on this, could this be expected to get merged before 5.0?

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Oct 4, 2018

Member

Yes, it's something we want to get in.

I've been working on other tasks but hopefully can focus on this again soon. At the very least it would be good to deprecate the REST API endpoint which I think is the only breaking API change involved.

Member

noisysocks commented Oct 4, 2018

Yes, it's something we want to get in.

I've been working on other tasks but hopefully can focus on this again soon. At the very least it would be good to deprecate the REST API endpoint which I think is the only breaking API change involved.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 18, 2018

Member

Splitting off the refactoring to remove WP_REST_Blocks_Controller in #10751

Member

aduth commented Oct 18, 2018

Splitting off the refactoring to remove WP_REST_Blocks_Controller in #10751

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