Site Editor: Deprecate edited entity state #66965
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Code Quality
youknowriad
requested review from
mcsf,
Mamaduka,
getdave,
jsnajdr and
tellthemachines
youknowriad
requested review from
ntwb,
nerrad,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
youknowriad
commented
Nov 13, 2024
| @@ -85,38 +89,25 @@ export default function EditSiteEditor( { isPostsList = false } ) { | |||
| const { canvas = 'view' } = params; | |||
| const isLoading = useIsSiteEditorLoading(); | |||
| useAdaptEditorToCanvas( canvas ); | |||
| const entity = useResolveEditedEntity(); | |||
There was a problem hiding this comment.
I think this hook and the current component can be simplified even further by moving this "route specific" logic to the routes directly. Routes know better whether they should just render the provided postType, postId or whether they should fetch the home page... (instead of trying to be smart and centralizing all the logic in a single hook like we do today).
That said, one downside of doing so is that "routes" will have different component trees (they need to call different hooks), which means that switching between these routes will unmount and remount the whole "editor" component. I'm not entirely sure how to solve that yet (to ensure smooth transitions...) that's why I didn't really touch useResolveEditedEntity in the current PR and potentially left this for later. cc @jsnajdr in case you have ideas there.
There was a problem hiding this comment.
To verify my understanding: here the edited entity is no longer resolved just once in useInitEditedEntityFromUrl and stored into the edit-site store, but it's resolved directly in the EditSiteEditor component? The context etc is no longer a value in data store, but just a local variable returned from a hook?
There was a problem hiding this comment.
I think this hook and the current component can be simplified even further by moving this "route specific" logic to the routes directly.
Does that mean that we would break up the hook into multiple ones, each handling a specific case? 1) post types without a parent template; 2) pages; 3) everything else? That's how the current (very convoluted) logic seems to be structured.
There was a problem hiding this comment.
Does that mean that we would break up the hook into multiple ones, each handling a specific case? 1) post types without a parent template; 2) pages; 3) everything else? That's how the current (very convoluted) logic seems to be structured.
Yes exactly, I want to try that next, but my fear is that the "remounting" is going to prevent me from actually doing it.
To verify my understanding: here the edited entity is no longer resolved just once in useInitEditedEntityFromUrl and stored into the edit-site store, but it's resolved directly in the EditSiteEditor component? The context etc is no longer a value in data store, but just a local variable returned from a hook?
You can see that I still call useSyncDeprecatedEntityIntoState after the "resolve" hook. That one is mostly there for backwards compatibility (people still accessing the state)
There was a problem hiding this comment.
my fear is that the "remounting" is going to prevent me from actually doing it.
Yes, instead of always rendering <Editor /> we'd have multiple components:
function TemplateEditor() {
const { postType, postId, templateId } = useResolveEditedTemplate();
return <Editor postType postId templateId />;
}
function PageEditor() {
const { postType, postId, templateId } = useResolveEditedPage();
return <Editor postType postId templateId />;
}and that causes remounting.
One fact that could help is that the resolution code doesn't need to be a hook, it can be an async function that calls a series of await resolveSelect.getEntityRecords() and returns a promise of resolved template + context. Then the editor component always calls the same hook but with a different "resolver":
const { postType, postId, templateId } = useResolve( templateFetch );
return <Editor {...} />;The downside of this is that the "resolver" is not reactive: if the edited entity slug changes, for example, the React component won't be notified and updated, although the resulting template depends on the slug.
There was a problem hiding this comment.
Oh this reminds of the concept of "loaders" :) Maybe we can introduce it in our routes and just have { postType, postId, context } = useLoaderData or something like that :P
|
@youknowriad, the CI check failures are valid. I'm seeing errors like: |
|
Size Change: -63 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Nov 14, 2024
Mamaduka
reviewed
Nov 14, 2024
Mamaduka
approved these changes
Nov 14, 2024
youknowriad
added
the
Needs Dev Note
jsnajdr
reviewed
Nov 14, 2024
|
|
||
| useEffect( () => { | ||
| if ( isReady ) { | ||
| resetZoomLevel(); |
There was a problem hiding this comment.
Was this resetZoomLevel call moved elsewhere or is it no longer needed?
There was a problem hiding this comment.
I saw that the original PR that introduced this was further refactored (removed some bits) but kept this call. I asked there to confirm, can restore later if needed but this definitely felt like the wrong place for such an action call.
| @@ -85,38 +89,25 @@ export default function EditSiteEditor( { isPostsList = false } ) { | |||
| const { canvas = 'view' } = params; | |||
| const isLoading = useIsSiteEditorLoading(); | |||
| useAdaptEditorToCanvas( canvas ); | |||
| const entity = useResolveEditedEntity(); | |||
There was a problem hiding this comment.
To verify my understanding: here the edited entity is no longer resolved just once in useInitEditedEntityFromUrl and stored into the edit-site store, but it's resolved directly in the EditSiteEditor component? The context etc is no longer a value in data store, but just a local variable returned from a hook?
| @@ -85,38 +89,25 @@ export default function EditSiteEditor( { isPostsList = false } ) { | |||
| const { canvas = 'view' } = params; | |||
| const isLoading = useIsSiteEditorLoading(); | |||
| useAdaptEditorToCanvas( canvas ); | |||
| const entity = useResolveEditedEntity(); | |||
There was a problem hiding this comment.
I think this hook and the current component can be simplified even further by moving this "route specific" logic to the routes directly.
Does that mean that we would break up the hook into multiple ones, each handling a specific case? 1) post types without a parent template; 2) pages; 3) everything else? That's how the current (very convoluted) logic seems to be structured.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #66921
What?
As the site editor is growing to become a multi page app, it doesn't really make sense to have an edited post type and context state in it.
That state is forcing us today to have a two way synchronization mechanism between the url and the state causing some issues related to performance and some hidden bugs at times.
Ideally the routes would just pass the post type, post id to the editor component. The current PR helps to go in that direction by deprecated the edited entity state selectors and actions that were remaining in the site editor store. This prevents developers from mistakenly using them in the future.
Testing Instructions