-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Edit Post: Automatically connect a menu item with the pinnable sidebar plugin #29081
Conversation
Size Change: +449 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
@jasmussen, it's nearly ready but I struggle with finding a good testing examples. I might also miss some use cases 😃 I will let you know again once I sort everything out. |
6901701
to
844ffef
Compare
It looks better with the latest changes applied: So the original issues seem to be resolved, but there are other issues more visible now:
|
953cd80
to
eff247c
Compare
}, | ||
__( 'Sidebar title plugin' ) | ||
__( 'Plugin sidebar more menu title' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the name to ensure that we use the version explicitly registered by the plugin, rather than the one automatically injected.
I think I found a way to fix the issue by using I think I have some ideas on how to fix the issue with the order of pinned items. It is less of an issue in the More Menu for the Plugins section and I believe it has the same root. My guess is that all pinned items get unmounted but the one that has currently opened sidebar. I will investigate separately as it isn't related to this PR at all. #28546 was created for a reason 😄 |
This is very cool :) Is |
Oh actually, that prop is useful, I misunderstood its purpose originally :) |
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
There needs to be a way to detect which fill should be preferred in More Menu when it gets duplicated. I didn't come up with anything nicer but if you can think of something more elegant I will willingly update the code :) I explained the reasons for the current implementation in the description this way:
|
I found what the issue is. It's a subtle difference in the implementation that regressed here: It's so simple that I will include it here. |
a5764b9
to
21a4059
Compare
</PinnedItems> | ||
) } | ||
{ name && isPinnable && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a prop that is mandatory and unique for the PluginSidebar
implementation. Would it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the name the prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's part of the PluginSidebar
API. In other places identifier
is used for some reasons.
<ComplementaryAreaMoreMenuItem | ||
// Menu item is marked with unstable prop for backward compatibility. | ||
// @see https://github.com/WordPress/gutenberg/issues/14457 | ||
__unstableExplicitMenuItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works well. Good idea @youknowriad 👏🏻
@youknowriad I think I was able to address your feedback: The last remaining thing I discovered now are two warnings that need to be fixed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good
Thank you so much for doing this! This is exactly how I was hoping it'd work. Note: the bigger button that's toggled, I will look at that separately. Another question: will #28546 be easier to address once this lands or is that entirely separate? |
This PR improves the order of pinned items - they don't change after unpinning/pinning anymore. I will look separately how to ensure that core features are always first on the list. |
I will work on further improvements in the follow-up PR. |
Description
Fixes #14457.
Fixes partially #28546.
When the plugin renders a pinnable
PluginSidebar
component, then by default it also automatically renders a correspondingPluginSidebarMenuItem
. It's respecting matchingPluginSidebarMenuItem
components registered by plugins and therefore is fully backward compatibleTechnical implementation
This PR adds Special handling for backward compatibility. It ensures that menu items created by plugin authors aren't duplicated with automatically injected menu items coming from pinnable plugin sidebars. In practice, we create two fills, one indirectly from
PluginSidebar
and another that the plugin's author defines withPluginSidebarMoreMenuItem
. At the time of rendering the Plugins section in MoreMenu, code iterates over all fills and detects possible duplications. When it happens then the fill rendered byPluginSidebar
gets removed.__unstableInitSource
prop was added to make it possible to decide which duplicated fill should be ignored.How has this been tested?
Case 1
Register a plugin with a sidebar only:
Case 2
The menu link for the sidebar should be added automatically in More Menu.
Register a plugin with a sidebar and a menu link:
The menu link from the plugin should be rendered in More Menu, but the link from the sidebar should be removed.
Screenshots
Types of changes
New feature and a bug fix at once? It's hard to tell really 😄
Checklist: