-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 content area of the post and site editors #61860
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. |
@@ -367,7 +364,9 @@ function Layout( { initialPost } ) { | |||
{ ( mode === 'text' || ! isRichEditingEnabled ) && ( | |||
<TextEditor /> | |||
) } | |||
{ ! isLargeViewport && <BlockToolbar hideDragHandle /> } | |||
{ ! isLargeViewport && mode === 'visual' && ( |
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.
Extra check to align with site editor.
@@ -377,9 +376,6 @@ function Layout( { initialPost } ) { | |||
<MetaBoxes location="advanced" /> | |||
</div> | |||
) } | |||
{ isMobileViewport && sidebarIsOpened && ( | |||
<ScrollLock /> |
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.
Seems like component was added a long time ago for mobile but it's not present in the site editor and I'm not sure why we need it. Maybe @jasmussen or @ellatrix would know if this removal is impactful or not.
<TemplatePartConverter /> | ||
<PatternModal /> | ||
</> | ||
) } |
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.
Most of these don't need to be in the "content" area. So I just moved them to align the content area with the post editor.
<> | ||
<TemplatePartConverter /> | ||
<PatternModal /> | ||
</> |
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 both of these template part converter and pattern modal components need to move to the editor package. They provide features that are site editor only for now for no obvious reasons.
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.
PatternModal
can definitely move IMO and also move the commands that use them. The template part converter though is probably because we do not allow inserting template parts in posts. Now that both editors are converging, I'm curious if the decision to exclude template parts in post editor should be revisited too..
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.
We do allow template parts in the template editor which is also available in the post editor. So it should move too.
For the pattern modal, I'm moving it here #61862
For the template part converter, I'm waiting for @jorgefilipecosta's PR that moves the template part creation modal first.
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.
Hi @youknowriad, that PR is available at #61879.
Size Change: -25 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
Flaky tests detected in 6a8505d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9191362704
|
@@ -273,6 +274,7 @@ export const ExperimentalEditorProvider = withRegistryProvider( | |||
{ type === 'wp_navigation' && ( | |||
<NavigationBlockEditingMode /> | |||
) } | |||
<EditorKeyboardShortcuts /> |
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.
Should we also move EditorKeyboardShortcutsRegister
?
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.
No, because we want the shortcuts to be registered regardless of whether they are actually applied. Registering the shortcuts allow them to be visible in the keyboard shortcuts modal.
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 still don't get how it would affect the modal if we moved them in Editor provider. Will do some testing to verify 😄
--edit. I tested and it works for me. Maybe I'm testing a different thing? I moved the register in editor/EditorProvider
and shortcuts 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.
It works right now because the keyboard shortcuts modal is rendered only when the editor is rendered but in theory the keyboard shortcuts modal should be accessible every time (even in the site editor shell).
I think it's something we can revisit though and the separation between registration and availability in the modal can be reconsidered.
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.
If we ever reconsider, I think we should just unify the components/registration...
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.
Besides the open question about ScrollLock
, this LGTM. Thanks!
There appears to be a small issue with this PR. The root padding broke, somehow: I found that out by checking the previous commit in https://github.com/WordPress/gutenberg/commits/trunk/?before=f49e0b5958adad9fbd211d5ded07b4cabf5bfec3+51, and that one is fine: |
Thanks for catching that @jasmussen I'll take a look. |
@jasmussen Fix here #61906 |
…s#61860) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
…s#61860) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Related #52632
What?
This is just a small PR that moves some components/elements around in order to prepare for a future potential unification of the whole "content" area of the editor skeleton between post and site editors.
Testing Instructions
There should be no functional change here.