-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Editor: Unify the sidebar between the post and site editors #61507
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -287,6 +288,57 @@ function Layout( { initialPost } ) { | |||
); | |||
} | |||
|
|||
const { createSuccessNotice } = useDispatch( noticesStore ); | |||
|
|||
const onActionPerformed = useCallback( |
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 callback is specific per editor at the moment. So I moved it into a prop to the "Sidebar" component. Ideally this should be the same between editors to be honest.
d75b491
to
661e171
Compare
}; | ||
}, [] ); | ||
return { | ||
blockPatterns: getSettings().__experimentalBlockPatterns, |
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'm not entirely sure that the way I'm retrieving the patterns here is similar to what we had before. Also not sure how to test this panel properly yet.
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.
Also I'm guessing this PR brings this panel to the post editor (where it was not present)
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.
There should be a simple way to get the patterns. Each similar implementation in the core has to deal with extra logic, increasing the chances of missing something.
@ellatrix, you recently worked on refactoring pattern fetching logic. Is there a better way to get all the patterns?
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.
For now, I just restored the code that we had in the site editor to avoid impact but I agree that overall, we need to have a better story about retrieving patterns.
661e171
to
04058d5
Compare
onActionPerformed={ onActionPerformed } | ||
extraPanels={ | ||
! isEditingPage && ( | ||
<PluginTemplateSettingPanel.Slot /> |
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.
The position of this slot has changed in the DOM. This slot is also deprecated, so I'm not sure we should make the effort to keep the previous position or not.
{ renderingMode !== 'post-only' && ( | ||
<TemplateContentPanel /> | ||
) } | ||
<PostTransformPanel /> |
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.
Both of these panels were site editor specific and I'm guessing they can show up in the post editor as well now.
04058d5
to
fcab8e0
Compare
Size Change: -1.43 kB (-0.08%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
PagesFor pages this adds the 'Move to trash' button to the Inspector in the Site Editor. Not a big deal, but noting since we'll want to remove that button as part of the Inspector work. Trashing/deleting should be consistently accessed from the ellipsis menu: Template partsTemplate parts receive the same Summary and trash button, plus a slug input: In this case the trash button is problematic because it appears for theme-supplied template parts (which cannot be deleted). The same is true for the slug. Once template parts and patterns are more unified (#57011) we'll want to include the Summary panel, but until then I think ideally it would be omitted in this context (as it is on trunk). PatternsThe Summary panel appears here too. Probably fine, but it's very short which highlights the need to move away from the collapsible panel component soon. NavigationAlso receives the Summary panel. It's not very useful here at all, containing only the 'last edited...' message and the 'Move to trash' button (which we want to remove anyway). Similar to template parts, if possible I think it's worth omitting this panel. |
@jameskoster, I think we should also remove the "Rename" action for normal post-types. The title is available in canvas and probably more accessible than a renaming in a modal. |
Do you mean posts and pages? It's not always the case that the title is on the canvas, for instance if you have template preview on, and the template doesn't include a Title block. I don't think it hurts to include it as a fallback, though perhaps "Edit title" would make more sense than "Rename" for posts and pages. |
19c6e44
to
f0f85ea
Compare
@jameskoster feedback addressed. |
b94561f
to
b995c53
Compare
fb6c61c
to
8436924
Compare
case 'move-to-trash': | ||
{ | ||
const postType = items[ 0 ].type; | ||
document.location.href = addQueryArgs( 'edit.php', { | ||
post_type: postType, | ||
} ); | ||
} | ||
break; |
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.
c9349cd
to
b80f5c6
Compare
@@ -33,30 +35,16 @@ import { store as editorStore } from '../../store'; | |||
const PANEL_NAME = 'post-status'; | |||
|
|||
export default function PostSummary() { | |||
const { isOpened, isRemoved, showPostContentPanels } = useSelect( |
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.
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.
It was suggested above that it's not very helpful #61507 (comment)
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.
To minimize changes, I restored it but kept only the "sync status"
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 we still want to display Sync status
somewhere. The header has a purple accent color, but that's not very accessible.
I don't think it's a blocker here as long as we quickly ship a follow-up for it (same Gutenberg release). cc @jameskoster
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.
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.
@jameskoster, is there an issue for that?
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.
Not for patterns specifically. It's a part of #59689.
b80f5c6
to
22e2ed0
Compare
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.
Thanks, @youknowriad!
This tests well for me, and I couldn't spot any regression.
It would be nice to get another testing review in case I missed something. There's a lot of nuance to panel rendering logic.
Happy to address any follow-ups that appear. |
@jameskoster, the "Post Actions" are disabled when viewing the template in the post editor. Screenshot |
I believe that occurs when actions (in this case 'Reset') cannot be applied. It's the same in the site editor. It may be better to hide the ellipsis button in these cases. |
I thought the action would be simply omitted or disabled when it's not eligible for the current entity. |
Yup, that's another option – keep the ellipsis active, but the Reset action in the menu disabled. |
Related #52632
Follow-up to #61497
closes #29407
What?
We're in a middle of an effort to merge the post and site editors under the @wordpress/editor abstraction. As part of this, this PR unifies the Sidebar component used in both post and site editors.
The structure of the two components was a bit different so there's a possibility for this changing the output a bit. So we need to test the sidebar of all post types (post, page, template, template part, navigation, pattern) properly to ensure the result is as we expect it to be.
Testing Instructions
Please test the sidebar (block and document) of the post and site editors.
There are some subtle changes to be expected but overall it should be similar to trunk.