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

Block Library: Reimplement Reusable Block using EditorProvider for embedded post editor #14715

Draft
wants to merge 4 commits into
base: master
from

Conversation

@aduth
Copy link
Member

aduth commented Mar 29, 2019

Closes #7119
Previously: #7453

This pull request seeks to reimplement the reusable block as an embedded post editor, using EditorProvider.

Work in Progress:

This pull request currently cherry-picks a few related fixes (#14711), and includes changes which are likely more appropriate to be proposed in their own pull requests. It also lacks sufficient test coverage and documentation for newly proposed features.

Functionally, it largely works as expected.

I'm still trying to find the right balance of the extent to which this should be refactored. Currently, I'm considering to not remove any actions around fetching or saving reusable blocks as it impacts the inserter and block static/reusable conversion features. To that end, it would largely be a refactor primarily only impacting the block itself, but leaving all other behaviors intact.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jun 4, 2019

I'm looking to refresh this soon, rebasing to resolve conflicts.

To note, the current commits b5c43e3 and 58c4a02 have been resolved separately by #14711. These will be dropped from the branch.

I plan to open separate, individual pull requests for the commits 35b8e8e and 53be3f2, which serve standalone purpose (consistency with BlockEditorProvider, also related to #15949).

@aduth aduth force-pushed the update/reusable-block-embedded-editor branch from 9ee5606 to bf73ab7 Jun 4, 2019
@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jun 4, 2019

I've finished and pushed the rebase here. Functionally it seems to be in working order again. There are two failing unit test in components/slot-fill/test/slot.js, which I expect are related to more recent refactorings in #15541. I started down the path of refactoring the proposed Slot/Fill withConsumerContext of 71c3ec2 to a custom hook instead (similar to @wordpress/data's useRegistry), but since this will require a refactor of the Slot component to a function component (required for hooks), I will postpone this effort and propose the function refactoring separately (which should be done anyways).

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jun 4, 2019

I plan to open separate, individual pull requests for the commits 35b8e8e and 53be3f2

See #15989 and #15988 respectively.

__experimentalBlockSettingsMenuFirstItem,
__experimentalBlockSettingsMenuPluginsExtension,
} from '@wordpress/block-editor';
import { EditorProvider } from '@wordpress/editor';

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

I would prefer if we could avoid the need to use the editor module in the reusable block. That would make us need to load the editor module in the widget screen, for example. It also makes our reusable block functionality very tied WordPress posts.
What if the BlockEditor settings accept a property that allows retrieving the blocks of the reusable block with a given id, and changing the blocks/title of a reusable block with a given id? The block would use these block editor settings to retrieve the information needed and would then instantiate a BlockEditor instead of an Editor.

This is not a blocker is just a future possibility.

This comment has been minimized.

Copy link
@aduth

aduth Jun 27, 2019

Author Member

What if the BlockEditor settings accept a property that allows retrieving the blocks of the reusable block with a given id, and changing the blocks/title of a reusable block with a given id? The block would use these block editor settings to retrieve the information needed and would then instantiate a BlockEditor instead of an Editor.

In what you're considering, where would you propose that the logic for saving / fetching reusable block as a post occur?

import { Toolbar, Slot, DropdownMenu } from '@wordpress/components';
import { Toolbar, Slot, DropdownMenu, createSlotFill } from '@wordpress/components';

const { Slot: ToolbarControlsSlot } = createSlotFill( 'RichText.ToolbarControls' );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

This change was only to have a reference to the slot so we can pass it to SlotFillProvider right?

This comment has been minimized.

Copy link
@aduth

aduth Jun 27, 2019

Author Member

This change was only to have a reference to the slot so we can pass it to SlotFillProvider right?

Yes, rather than reference these by their string names (which so far we've considered an internal implementation detail), it seems we may instead want to reference the components themselves.


if ( Array.isArray( handledSlots ) ) {
const isHandled = some( handledSlots, ( slotNameOrComponent ) => {
const { slotName = slotNameOrComponent } = slotNameOrComponent;

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

This works well and is smart but makes the code a little hard to follow. Maybe we can just assign the name with const slotName = ( typeof slotNameOrComponent === 'string' ) ? slotNameOrComponent : slotNameOrComponent.slotName;
More verbose but I guess it is easier to follow.

This comment has been minimized.

Copy link
@aduth

aduth Jun 27, 2019

Author Member

This works well and is smart but makes the code a little hard to follow.

Yeah, in retrospect I think you're right that I may have tried to be a little too clever here. Your simpler alternative seems to be an improvement.

}
}

return handler.call( this, name, ...args );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

I'm probably missing something but is .call really required? Couldn't we just execute return handler( name, ...args );. Why do we have a need to change the "owner object"?

This comment has been minimized.

Copy link
@aduth

aduth Jun 27, 2019

Author Member

I'm probably missing something but is .call really required? Couldn't we just execute return handler( name, ...args );. Why do we have a need to change the "owner object"?

I don't immediately recall why or if this was necessary. Looking at it now, it feels like it may have been at least necessary that a this context exist in the component instance function being called, but as I see it here, it's probably wrong to consider it the this of the current instance (rather than the ancestor's), and I would think even without .call, the this would still be assigned in the ancestor (since most components define instance properties with .bind in the constructor).

@jorgefilipecosta jorgefilipecosta force-pushed the update/reusable-block-embedded-editor branch from bf73ab7 to edaf84f Jun 17, 2019
@jorgefilipecosta jorgefilipecosta added this to Generic Block Editor Issues in Widgets Jun 20, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Jun 20, 2019

I spent some time testing this PR. Here are some regressions I found (nothing major):

  • There is no way to select the reusable block title right after the creation. It always saves with the default title, without letting the user do any input after the creation. After it is saved the user can press the edit and edit the tile.
  • If we select a block inside a reusable block the inspector still says the selected block is the reusable block. It correctly shows the settings of the selected block.
  • If we multi-select blocks inside a reusable block, the inspector does not show a multi-select indication. I think we can solve this problem and the previous problem using a new slot that contains all the block inspector UI e.g: a slot where we render , similar to what I did in #16203.
  • It is not possible to insert a reusable block inside another reusable block.
  • If we have a block selected inside the reusable block editor and we insert a block using the global inserter the block gets inserted after the reusable block and not inside the reusable block. It is a complex to solve problem, the behavior in master is even worst -- nothing happens when the user tries to insert using the global inserter and has a block selected.

To note that even with this changes the reusable block still depends on the editor for the fetching of reusable blocks. But I guess it is something we can address after this PR.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Jun 27, 2019

I spent some time testing this PR. Here are some regressions I found (nothing major):

Thanks for the detailed review! I'll look through these issues and see what can be done to improve them.

To note that even with this changes the reusable block still depends on the editor for the fetching of reusable blocks. But I guess it is something we can address after this PR.

Yes, after a few failed attempts to switch everything over in the same pull request, I decided it would probably be best to do it in a few steps instead.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Nov 4, 2019

There is likely significant refactoring necessary to bring this up to date with respect to the introduction of "entity providers" (#17368).

@aduth aduth mentioned this pull request Dec 16, 2019
5 of 6 tasks complete
@aduth aduth mentioned this pull request Jan 3, 2020
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Widgets
Generic Block Editor Issues
2 participants
You can’t perform that action at this time.