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

Replace the renderBlockMenu prop with a Slot #7199

Merged
merged 2 commits into from Jun 11, 2018

Conversation

3 participants
@youknowriad
Contributor

youknowriad commented Jun 7, 2018

Related #7192

This PR replaces the renderBlockMenu Prop with Slot/Fill.

Testing instructions

  • Open the block menu and toggle the inspector (Show Block Settings)
  • It should work.

@youknowriad youknowriad self-assigned this Jun 7, 2018

@youknowriad youknowriad requested review from gziolo and aduth Jun 7, 2018

count === 1 && <SharedBlockSettings key="shared-block" uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" />,
<BlockTransformations key="transformations" uids={ uids } onClick={ onClose } itemsRole="menuitem" />,
] } ) }
<PluginsBlockSettingsMenuSlot fillProps={ { onClose } } />

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

Questions:

  • This adds the menu items to the top of the list, should we add another slot after the menu items?
  • We don't use any prop (aside onClose) in our current usage, but I guess if we want to expose this to plugins, we'd probably need uids?

Thoughts?

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

This adds the menu items to the top of the list, should we add another slot after the menu items?

I don't know what is the exact use case in here, but I don't feel very good about having two slots only to have a way to pick whether we should prepend or append menu items. I'm wondering if we shouldn't rethink the way all slot operate. There are a few things we can explore. First of all, we could convert all menu items into fills to have better controls over them and:

  • Introduce priority prop for fills.
  • Add a filter for a slot which would allow reordering or modifications of fills.

...or maybe use a filter instead of Slot/Fill in the first place.

In general, there is an expectation from plugin developers to have a way to remove some of the fills that are rendered. We can provide an API for that, or find a way to guard fills with the check which allows preventing their rendering if the given condition is met.

We don't use any prop (aside onClose) in our current usage, but I guess if we want to expose this to plugins, we'd probably need uids?

count might make sense, too.

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

Yes, I agree it's not perfect, Slots are limited to specific positions. A priority/order support makes sense. That said, I think it's a bigger problem than what this PR is trying to solve and it concerns all the other slots.

Like you said, I think we should move on like this, for now, it's way better than the renderBlockMenu prop and address the slots flexibility in a dedicated issue/PR.

@@ -86,8 +86,7 @@ export class BlockListBlock extends Component {
}
createInnerBlockList( uid ) {
const { renderBlockMenu } = this.props;
return createInnerBlockList( uid, renderBlockMenu );
return createInnerBlockList( uid );

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

I didn't want to refactor further here because it's taken care of in #7192

This comment has been minimized.

@aduth

aduth Jun 11, 2018

Member

I didn't want to refactor further here because it's taken care of in #7192

By refactoring, do you include removal of the unused-as-of-these-changes renderBlockMenu argument of createInnerBlockList?

@gziolo

In general, I like the flexibility Slot & Fill gives, but there are a few limitations I described in my detailed comment. It helps to remove renderBlockMenu from the BlockEditContext which is great. If it works exactly as before, I would proceed. @aduth should have a much better idea if it fits.

@@ -23,6 +23,11 @@ import BlockTransformations from './block-transformations';
import SharedBlockSettings from './shared-block-settings';
import UnknownConverter from './unknown-converter';
const { Fill: PluginsBlockSettingsMenu, Slot: PluginsBlockSettingsMenuSlot } =

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

Nit: it should land in its own file.

This comment has been minimized.

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

I personally don't like the pattern of assigning the .Slot into the Fill because we don't want to expose the Slots but I'm following the pattern now, we can change that later.

This comment has been minimized.

@gziolo

gziolo Jun 8, 2018

Member

It is what it is, I followed what was in code before ...

count === 1 && <SharedBlockSettings key="shared-block" uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" />,
<BlockTransformations key="transformations" uids={ uids } onClick={ onClose } itemsRole="menuitem" />,
] } ) }
<PluginsBlockSettingsMenuSlot fillProps={ { onClose } } />

This comment has been minimized.

@gziolo

gziolo Jun 7, 2018

Member

This adds the menu items to the top of the list, should we add another slot after the menu items?

I don't know what is the exact use case in here, but I don't feel very good about having two slots only to have a way to pick whether we should prepend or append menu items. I'm wondering if we shouldn't rethink the way all slot operate. There are a few things we can explore. First of all, we could convert all menu items into fills to have better controls over them and:

  • Introduce priority prop for fills.
  • Add a filter for a slot which would allow reordering or modifications of fills.

...or maybe use a filter instead of Slot/Fill in the first place.

In general, there is an expectation from plugin developers to have a way to remove some of the fills that are rendered. We can provide an API for that, or find a way to guard fills with the check which allows preventing their rendering if the given condition is met.

We don't use any prop (aside onClose) in our current usage, but I guess if we want to expose this to plugins, we'd probably need uids?

count might make sense, too.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 11, 2018

Can you see this , (comma) in the menu?
screen shot 2018-06-11 at 14 18 44

it works properly for nested blocks

{ count === 1 && <UnknownConverter uid={ firstBlockUID } role="menuitem" /> }
<BlockDuplicateButton uids={ uids } rootUID={ rootUID } role="menuitem" />
{ count === 1 && <SharedBlockSettings uid={ firstBlockUID } onToggle={ onClose } itemsRole="menuitem" /> }
<BlockTransformations uids={ uids } onClick={ onClose } itemsRole="menuitem" />,

This comment has been minimized.

@gziolo

gziolo Jun 11, 2018

Member

Found this , :D

This comment has been minimized.

@youknowriad
const { Fill: PluginBlockSettingsMenu, Slot } = createSlotFill( 'PluginsBlockSettingsMenu' );
PluginBlockSettingsMenu.Slot = Slot;

This comment has been minimized.

@gziolo

gziolo Jun 11, 2018

Member

We should document it somewhere. In general, we should document all Slot/Fill pairs from editor. Let's open a follow-up task.

This comment has been minimized.

@youknowriad

youknowriad Jun 11, 2018

Contributor

yes, I was not sure if we wanted to document it now or wait until we figure out #7199 (comment)

@gziolo

gziolo approved these changes Jun 11, 2018

I tested it locally and it works as expected. The comma needs to be removed and it's a very good improvement.

@gziolo gziolo added this to To do in Extensibility via automation Jun 11, 2018

@gziolo gziolo added the Blocks label Jun 11, 2018

@gziolo gziolo added this to the 3.1 milestone Jun 11, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jun 11, 2018

I don't know what is the exact use case in here, but I don't feel very good about having two slots only to have a way to pick whether we should prepend or append menu items. I'm wondering if we shouldn't rethink the way all slot operate. There are a few things we can explore. First of all, we could convert all menu items into fills to have better controls over them and:

  • Introduce priority prop for fills.
  • Add a filter for a slot which would allow reordering or modifications of fills.

...or maybe use a filter instead of Slot/Fill in the first place.

While it's tempting to give developers full control over these items so far as the flexibility it provides, to me it seems like this isn't something a developer should want to be concerned with, and in its impact on opening the potential for inconsistent user experience (or even abuse), not something that has a net benefit for anyone involved.

Ultimately, what is a developer trying to do, and in what ways does order matter? For plugin developers, it seems like the mere presence in the list of options should be sufficient (though I'm open to hearing use-cases to the contrary). The complication for us here is that "Show Block Settings" is part of a core (post editor) behavior, and we do agree its sensible to be shown in a specific order (i.e. first in the list).

So how do we distinguish between:

  • Core behavior menu options vs. plugin extensions
    • Does this distinction need to be presented in the UI for the user? (e.g. a line separator, like with "Transform into:")
    • I tend to dislike special treatment of core, but this seems one area where its sensible to the user to perceive where a line is drawn.
  • What are the core behaviors that ought to be implemented as extensions
    • All of them?
  • Does order matter?
    • Would it be bad if "Show Block Settings" was not the first option? (Maybe yes)

Among plugin extensions proper, I personally don't think it's in the interest of anyone to introduce a concept of ordering. A single slot should be sufficient. To the user, its order in the list should be consistent (deterministic), but aside from that, I'd not care what metric is used for ordering.

For core behaviors, maybe it should be a separate slot from that of plugins, which prompts other questions:

  • How do avoid plugin authors from accessing it? Awkward and obvious naming akin to dangerouslySetInnerHTML / __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED ? Somehow prevent it from being exposed at all?
  • How do assure "Show Block Settings" as the first menu item? It's odd to me that this is both an extension, but one which the editor is expected to respect as a primary item. I could imagine even a third slot for a single "critical" fill, if we'd agree that other options don't have as much importance to their specific ordering. If we do want a fixed order, then maybe your suggestion of a priority on a fill could be a valid option to explore. But toward general usage, it introduces the same concerns about what ordering a plugin ought to be concerned with (of course I think my plugin menu option should show first, so I'm going to set priority={ -Infinity }).
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 11, 2018

Good thoughts @aduth we're often tricked by the fact developers want to extend everything for better and for worse. And often for worse in terms of UX for the user.

What's the way forward here, should we just rename the slot something like __CoreBlockSettingsMenu?

@aduth

This comment has been minimized.

Member

aduth commented Jun 11, 2018

@youknowriad There's precedent here with core PHP function naming (e.g. _set_preview).

This function’s access is marked private. This means it is not intended for use by plugin or theme developers, only in other core functions. It is listed here for completeness.

I'd prefer if it was not available at all, though I cannot think of a solution for this currently. Seems reasonable to have as a naming convention for now, and since it's intended to be private, can be open for future backwards-incompatible refactoring.

@aduth

This comment has been minimized.

Member

aduth commented Jun 11, 2018

But there's still a question: Even for core behaviors, would the expected behavior of the slot be to append or prepend? Do we need to reflect this in the name of the slot? Something like _BlockSettingsMenuFirstItem ? 🤷‍♂️

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 11, 2018

I think _BlockSettingsMenuPrepend is more accurate?

@aduth

This comment has been minimized.

Member

aduth commented Jun 11, 2018

I guess I don't like thinking of these as prepend and append behaviors, but rather within slots, each fill should be agnostic to its ordering. This is a strong "IMO" and is partly influenced by the complications which result by trying to introduce ordering / prioritization (though elsewhere we've relented, e.g. core block registration and transforms). Where the menu is effectively a set of slots / default items:

+------------------------------+
| BlockSettingsMenuFirstItem   |
| BlockSettingsMenu            |
| PluginBlockSettingsMenu      |
| (Transforms)                 |
+------------------------------+
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 11, 2018

My reasoning is that if we use it twice to another UI button, it won't act as "First Item"

@aduth

This comment has been minimized.

Member

aduth commented Jun 11, 2018

At that point (and only if it's considered reasonable) we ought to consider whether a formal ordering system should be implemented.

@youknowriad youknowriad merged commit 3504ae7 into master Jun 11, 2018

2 checks passed

codecov/project 46.71% (+0.02%) compared to 681aaed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from To do to Done Jun 11, 2018

@youknowriad youknowriad deleted the update/replace-render-block-menu-with-slot branch Jun 11, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 12, 2018

At that point (and only if it's considered reasonable) we ought to consider whether a formal ordering system should be implemented.

We could try to implement it by leveraging filters from wp.hooks as an internal mechanism to calculate the order of fills.

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