Site Editor: use EditorProvider instead of custom logic #56000
Conversation
| @@ -1,114 +0,0 @@ | |||
| /** | |||
| * WordPress dependencies | |||
| */ | |||
There was a problem hiding this comment.
@draganescu I'm trying to absorb the logic that we had here within the EditorProvider of @wordpress/editor. Can you help ensure I didn't break anything for the navigation pages in the site editor.
| @@ -1,75 +0,0 @@ | |||
| /** | |||
There was a problem hiding this comment.
The logic that was here need to be absorbed by the EditorProvider from the @wordpress/editor package so I'm trying to add a templateMode prop there to support that. I added a prop but I'm not there yet, I first need to understand the logic and what are all the modes, how to trigger them...
| return defaultEditorSettings; | ||
| } | ||
|
|
||
| export default function useSiteEditorSettings() { |
There was a problem hiding this comment.
I hope I can remove this function entirely but we can only do so when all of the site editor is relying on EditorProvider and/or the BlockEditorProvider usage within the site editor doesn't need the extra settings provided in the EditorProvider
| @@ -142,35 +161,7 @@ export default function Editor( { listViewToggleElement, isLoading } ) { | |||
| const secondarySidebarLabel = isListViewOpen | |||
| ? __( 'List View' ) | |||
| : __( 'Block Library' ); | |||
| const blockContext = useMemo( () => { | |||
There was a problem hiding this comment.
All this logic has been moved directly to the EditorProvider component. It's going to benefit the post editor as well.
| type={ editedPostType } | ||
| id={ editedPostId } | ||
| > | ||
| <BlockContextProvider value={ blockContext }> |
There was a problem hiding this comment.
Here I basically just replaced these three providers with one EditorProvider
| * | ||
| * @param {string} navigationBlockClientId ClientId. | ||
| */ | ||
| function useForceFocusModeForNavigation( navigationBlockClientId ) { |
There was a problem hiding this comment.
The assumption here is that for all editors that render a "navigation" post type, we want to put the block in "contentOnly" mode.
| { id } | ||
| ); | ||
| const actualBlocks = useMemo( () => { | ||
| if ( type === 'wp_navigation' ) { |
There was a problem hiding this comment.
The assumption here is that for all editors that render a "navigation" post type, we want to create a wrapper block to ensure everything is rendered properly. The wrapper is "locked" but we can edit its content in contentOnly mode.
youknowriad
added
the
[Type] Code Quality
|
Size Change: +119 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a3eed70e4b3ca20aee08e9189bfd3381d4611ca5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6847046823
|
There was a problem hiding this comment.
There's something I couldn't yet work out when testing. When opening a page from the Design menu like this (don't edit anything, just navigate to a page and try to edit):
2023-11-10.14.58.09.mp4
The editor.BlockEdit filters are not firing, namely the ones that determine the content edit mode in withDisableNonPageContentBlocks.
The consequence is that I can't edit the post-content.
I didn't get to the bottom of it, but I noticed that when first editing a page this way, it looks like the blocks array returned from useEntityBlockEditor() is empty in edit-template, so maybe something to do with the edited record in that hook. Sorry I ran out of time!
Tricky thing is that if I open another template first, then navigate to a page to edit, then it's fine so I can only guess some context is loading at this stage. 🤔
| export const ExperimentalEditorProvider = withRegistryProvider( | ||
| ( { | ||
| __unstableTemplate, | ||
| templateMode = 'all', | ||
| post, |
There was a problem hiding this comment.
Just noticed while testing editing pages in the editor that the post object isn't resolved in time so the properties id and type might need some optional chaining or something.
youknowriad
force-pushed
the
unify/editor-provider
branch
from
2f7c363
to
ac35c02
Compare
There was a problem hiding this comment.
i tested the change only around the navigation section of the site editor (this PR removes the navigation provider, which is great) - so far things work as expected with one exception:
- make sure to have one navigation block which only contains a page list
- navigating to site editor -> navigation
- selecting the navigation with page list
- clicking edit
- the page list block when selected shows two edit buttons for some reason
Screenshot
|
@draganescu I haven't been able to replicate the duplicated "edit" button. |
|
@ramonjd I think there's a pre-existing issue with these filters. I wonder if it's a bug in But Also, the way we're adding and removing these filters dynamically doesn't feel great to me to be honest. |
|
The last failing test is one that you've added @draganescu (focus loss on template part) I haven't been able to understand why it fails in this PR. |
youknowriad
requested review from
kevin940726,
Mamaduka and
ellatrix
as code owners
There was a problem hiding this comment.
Just catching up, and I see that 572dd72 removes the logic for the hidden template mode introduced in #52674 that grabs the real blocks used within the template. This means that the hidden template mode no longer shows the post featured image block if present (and doesn't factor in the order in which the content blocks appear in the template):
| Trunk | This PR |
|---|---|
 |
 |
Would it be worth preserving that behaviour so that the content matches the order used in the template? I think the main benefit of the prior behaviour is probably the visibility of the featured image block when present in the template.
Other than that, this is testing well for me so far!
| postType, | ||
| postId, |
There was a problem hiding this comment.
Tiny nit: these are both still returned by the useSelect even though they're not retrieved in this destructuring. Is it worth removing them from within the useSelect callback, too (lines 133 and 134), since the corresponding lines are now handled in useSiteEditorSettings?
| { hasLoadedPost && ! editedPost && ( | ||
| <Notice status="warning" isDismissible={ false }> | ||
| { __( | ||
| "You attempted to edit an item that doesn't exist. Perhaps it was deleted?" | ||
| ) } | ||
| </Notice> | ||
| ) } |
There was a problem hiding this comment.
Moving this Notice changes where in the UI it's displayed. It now sits above the interface (including both sidebars), rather than within the editor canvas. Is that intentional? It looks decent to me, and I don't have a strong opinion, but thought I'd mention it since I wasn't sure if this PR was meaning to make visual changes 🙂
I'm not too sure how to trigger this warning to display manually, so locally I just made the condition true in order to render it:
| Before | This PR |
|---|---|
 |
 |
There was a problem hiding this comment.
That was not my intention. How did you reproduce this? For me that message shows up only if the canvas doesn't show up (! editedPost) condition. So maybe I didn't understand it properly.
There was a problem hiding this comment.
I couldn't actually get it to fire manually through real means, so my screenshot is somewhat artificial (via removing the condition). If it's looking fine for you when ! editedPost is true, then that sounds good to me!
I can restore it (with a bit of code complexity) but to be honest, I'm not sure we should do, or either why this mode should be different than the post editor? In the post editor, the featured image is visible in the sidebar instead? |
youknowriad
force-pushed
the
unify/editor-provider
branch
from
53fdf08
to
0a97b5a
Compare
|
I've restored the template preview mode like trunk (extract the blocks from the template) in c34e322. You can see the complexity in the commit and why I'm not sure it's worth it but I'm definitely ok with it for now. |
|
I bootstrapped some documentation about the
With that I think the PR is ready to be considered for merge. |
There was a problem hiding this comment.
Great work!
The site editor is working as expected for me:
✅ I can edit navigation post types
✅ Editing a page I can perform expected actions: toggling template editing mode, swapping templates.
✅ Template previews appear as expected. I can edit a page's template and the changes persist between preview modes (template hidden and displayed)
✅ Command center commands look good: testing editing template, editing various templates/pages
Also can't find any regressions when viewing or editing styles, patterns or templates.
Would be good to get a second review though.
You can see the complexity in the commit and why I'm not sure it's worth it but I'm definitely ok with it for now.
To maintain consistency between template and page editing in the site editor, from a user-facing perspective, I'd say it's worth maintaining it. There might be a case for a refactor, or even a review of the feature in general. It's pretty buried.
| const { getEditedPostContext, getEditedPostType, getEditedPostId } = | ||
| select( editSiteStore ); | ||
| const { getCanvasMode, getPageContentFocusType } = unlock( | ||
| select( editSiteStore ) | ||
| ); | ||
| const { getEditedEntityRecord, hasFinishedResolution } = | ||
| select( coreStore ); | ||
| const { __experimentalGetGlobalBlocksByName } = | ||
| select( blockEditorStore ); | ||
| const _context = getEditedPostContext(); | ||
| const _postType = getEditedPostType(); | ||
| const queryArgs = [ | ||
| 'postType', | ||
| getEditedPostType(), |
There was a problem hiding this comment.
| getEditedPostType(), | |
| _postType, |
| * | ||
| * @param {Array} post Block list. | ||
| * @param {boolean} template Whether the page content has focus (and the surrounding template is inert). If `true` return page content blocks. Default `false`. | ||
| * @param {boolean} mode Rendering mode. |
| for ( let i = 0; i < blocks.length; i++ ) { | ||
| // Since the Query Block could contain PAGE_CONTENT_BLOCK_TYPES block types, | ||
| // we skip it because we only want to render stand-alone page content blocks in the block list. | ||
| if ( [ 'core/query' ].includes( blocks[ i ].name ) ) { |
There was a problem hiding this comment.
Optional: remove premature multiblock checking and replace with string comparison, e.g., 'core/query' === blocks[ i ].name
There was a problem hiding this comment.
Wonderful, great follow-ups @youknowriad, thanks for preserving the behaviour on trunk for now and for adding in the extra documentation 👍
Nothing to add from me beyond what @ramonjd already mentioned. All testing nicely:
✅ Navigation editing is working correctly
✅ All site, template, page, post content with and without template preview modes are working correctly
✅ Code changes look good
You can see the complexity in the commit and why I'm not sure it's worth it but I'm definitely ok with it for now.
Yes, I see what you mean. Fortunately it looks like it'll be fairly straightforward to remove in follow-ups if we decide to scale back / simplify or remove the template preview toggle. As Ramon mentioned, it could be good to review the feature in general and figure out the desired level of prominence 👍
LGTM! ✨
| * | ||
| * @return {Array} Flattened object. | ||
| */ | ||
| function extractedPageContentBlockTypesFromTemplateBlocks( blocks ) { |
There was a problem hiding this comment.
Tiny nit (feel free to ignore since this function instead exported): would the imperative extract be clearer than extracted?
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
|
Thanks folks for the reviews, I appreciate it. |
|
Looks like this PR made the site editor navigation 3 or 4 times better and slowed a little bit the initial loading. I call that a win. |
youknowriad
removed
the
[Type] Code Quality
bph
added
the
Needs Dev Note
|
The dev note should clarify that now the "editor" store is actually filled with a real entity and not just an empty useless store. |
|
I think this has introduced a regression for the list view when editing a Navigation entity in the site editor, or when rearranging blocks within the |
Related to #52632
Follow up to #55970
What?
Refer to the discussion in #52632 for the reasoning. The ultimate goal is to be able to reuse more UI components (maybe the whole editor) between edit-post and edit-site.
So this PR updates the site editor to use
EditorProviderinstead of its customBlockEditorProviderusage.The biggest impact is going to be on three things which we'll have to test properly: