Add mechanism to render complementary areas on edit site #21430
Conversation
|
Size Change: +1.35 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
jorgefilipecosta
added
the
[Status] In Progress
jorgefilipecosta
force-pushed
the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
2 times, most recently
from
54587e1 to
e979501
Compare
jorgefilipecosta
removed
the
[Status] In Progress
|
|
||
| ### as | ||
|
|
||
| An array of two components the first is the component used on the container of the fills and the second is the component used to render each fill. |
There was a problem hiding this comment.
Noting that in all other places, you pass only one component/tag name with as prop.
I have some reservations about this approach, while I fully support the idea of having a way to change the container element with as. It feels very limiting that the slot decides how to render fills. Plugin authors should have more freedom about how the fills are wrapped. It's also for convenience when they want to build a wrapper component for a single ActionItem to avoid code duplications like
const MyActionItem = ( props ) => <ActionItem as={ MenuItem } foo="bar" { ... } />;It's still the same fill, but you would have to explicitly pass it to the slot.
There was a problem hiding this comment.
It feels very limiting that the slot decides how to render fills
The slot is the component that is aware of the ideal fills to render, e.g: if the slot wraps things with ButtonGroup the ideal fill is Button, if the slot wraps things with MenuGroup the ideal fill is MenuItem, if the slot wraps things with ToolbarGroup the ideal fill is ToolbarButton.
But I understand your point it removes freedom from the fill. I updated the code to keep flexibility on the fill. Right now we allow the slot to set a default component to use in the fills but we also allow an as prop on the fill, in case a different component is desired.
| return ( | ||
| <Fill name={ name }> | ||
| { ( fillProps ) => { | ||
| const { onClick: fpOnClick, as: Item } = fillProps; |
There was a problem hiding this comment.
I could not call it onClick again because we already have a onClick on the parent function, so I called it fpOnClick, "fp" comes from fillProps.
| - Type: `String` | ||
| - Required: Yes | ||
|
|
||
| ### complementaryAreaIdentifier |
There was a problem hiding this comment.
I see this prop name repeated in a few places, can it be shortened to identifier?
Is there any way to remove this prop in favor of the existing name?
There was a problem hiding this comment.
I updated to use identifier. The name prop is still supported but name props just includes the "sidebar name" without including the plugin name identifier is the full identifier of a complementary area.
|
|
||
| ComplementaryAreaWrapped.Slot = ComplementaryAreaSlot; | ||
| ComplementaryAreaWrapped.Toggle = ComplementaryAreaToggle; |
There was a problem hiding this comment.
I saw comments from @youknowriad in other places where he discouraged using this notation with components. I agree with that and I wanted to list some of the reasons why it isn't idea:
- it prevents dead-code elimination
- it makes it harder to apply higher-order components on the original component (
ComplementaryAreaWrappedc) - it groups a few independent components in one file
There was a problem hiding this comment.
I removed the usage of this notation.
| ) | ||
| } | ||
| </Slot> | ||
| <ActionItem.Slot |
There was a problem hiding this comment.
Doesn't it make code harder to follow? The existing implementation is very simple so maybe we can leave it as is.
If we were to leave it as is, shouldn't we propagate scope to the slot?
There was a problem hiding this comment.
Doesn't it make code harder to follow? The existing implementation is very simple so maybe we can leave it as is.
I reverted to the previous implementation 👍
gziolo
added
[Package] Interface
jorgefilipecosta
force-pushed
the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
from
e979501 to
24f0dd5
Compare
|
Hi @gziolo thank you for the review! I applied some changes to address the feedback you provided. |
jorgefilipecosta
force-pushed
the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
from
24f0dd5 to
12b0fab
Compare
|
This PR was rebased is ready for a review. |
jorgefilipecosta
force-pushed
the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
from
12b0fab to
de16e52
Compare
Squashed commits: [ff91accb86] Add mechanism to render complementary areas on edit site
jorgefilipecosta
force-pushed
the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
from
de16e52 to
57e5bad
Compare
jorgefilipecosta
deleted the
add/way-to-render-unpinned-complemetary-areas-on-edit-site
branch
| return ( | ||
| <ComponentToUse | ||
| icon={ selectedIcon && isSelected ? selectedIcon : icon } | ||
| isSelected={ isSelected } |
There was a problem hiding this comment.
It looks like this is causing an error in master:
Warning: React does not recognize the `isSelected` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `isselected` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
in button (created by ForwardRef(Button))
in Tooltip (created by ForwardRef(Button))
in ForwardRef(Button) (created by ComplementaryAreaToggle)
in ComplementaryAreaToggle (created by Context.Consumer)
in WithPluginContext(ComplementaryAreaToggle) (created by ComplementaryArea)
in div (created by SlotComponent)
in SlotComponent (created by Context.Consumer)
in Slot (created by Slot)
in Slot (created by PinnedItemsSlot)
in PinnedItemsSlot (created by Header)
in div (created by Header)
in div (created by Header)
in Header (created by Layout)
in div (created by InterfaceSkeleton)
in div (created by InterfaceSkeleton)
in InterfaceSkeleton (created by NavigateRegions(InterfaceSkeleton))
in div (created by NavigateRegions(InterfaceSkeleton))
in NavigateRegions(InterfaceSkeleton) (created by Layout)
in div (created by FocusReturnProvider)
in FocusReturnProvider (created by Layout)
in Layout (created by Editor)
in ErrorBoundary (created by Editor)
in BlockEditorProvider
in Unknown (created by Context.Consumer)
in WithRegistryProvider(BlockEditorProvider) (created by EditorProvider)
in BlockContextProvider (created by EditorProvider)
in EntityProvider (created by EditorProvider)
in EntityProvider (created by EditorProvider)
in EditorProvider (created by WithDispatch(EditorProvider))
in WithDispatch(EditorProvider)
in Unknown (created by WithSelect(WithDispatch(EditorProvider)))
in WithSelect(WithDispatch(EditorProvider))
in Unknown (created by Context.Consumer)
in WithRegistryProvider(WithSelect(WithDispatch(EditorProvider))) (created by Editor)
in div (created by DropZoneProvider)
in DropZoneProvider (created by Editor)
in SlotFillProvider (created by SlotFillProvider)
in SlotFillProvider (created by Editor)
in StrictMode (created by Editor)
in Editor (created by WithDispatch(Editor))
in WithDispatch(Editor)
in Unknown (created by WithSelect(WithDispatch(Editor)))
in WithSelect(WithDispatch(Editor)) react-dom.js:539:32
|
@noisysocks Related: #22967 |
Description
This PR adds a more menu for plugins in the edit site and implements functionality for this in the interface package.
The PR introduces an "ActionItem" slot fill mechanism for cases where we have a container normally controlled by the slot and we want to add items to the container using fills if no fill is added the container is not rendered.
This mechanism allows the slot to specify the components used to render the container and each item (Default to Button, ButtonGroup but can be changed to MenuItem, MenuGroup, div, snap or even custom components).
The PinnediItems area was refactored to use the ActionItem mechanism the more menu also uses it.
We also implement ComplementaryAction.Toggle that renders a component to toggle a complementary area. All current ways to toggle a complementary area (pinned items, more menu, and the close button) were refactored to use this component.
And to have something similar to PluginSidebarMoreMenuItem we implement ComplementaryAction.MoreMenuItem that allows plugins to have a simple way to add an action to the more menu that toggles their complementary areas.
Existing edit-post code was refactored to use the new functionality from the interface package.