Skip to content
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

Resets the Zoom on viewed/edited entity change #66232

Merged
merged 2 commits into from
Oct 21, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/edit-site/src/components/editor/index.js
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ import { privateApis as routerPrivateApis } from '@wordpress/router';
import { store as preferencesStore } from '@wordpress/preferences';
import { decodeEntities } from '@wordpress/html-entities';
import { Icon, arrowUpLeft } from '@wordpress/icons';
import { store as blockEditorStore } from '@wordpress/block-editor';

/**
* Internal dependencies
@@ -153,6 +154,9 @@ export default function EditSiteEditor( { isPostsList = false } ) {
[ settings.styles, canvasMode, currentPostIsTrashed ]
);
const { setCanvasMode } = unlock( useDispatch( editSiteStore ) );
const { __unstableSetEditorMode, resetZoomLevel } = unlock(
useDispatch( blockEditorStore )
);
const { createSuccessNotice } = useDispatch( noticesStore );
const history = useHistory();
const onActionPerformed = useCallback(
@@ -261,6 +265,11 @@ export default function EditSiteEditor( { isPostsList = false } ) {
tooltipPosition="middle right"
onClick={ () => {
setCanvasMode( 'view' );
__unstableSetEditorMode(
'edit'
);
resetZoomLevel();

// TODO: this is a temporary solution to navigate to the posts list if we are
// come here through `posts list` and are in focus mode editing a template, template part etc..
if (
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@ import { useEffect, useMemo } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { store as coreDataStore } from '@wordpress/core-data';
import { privateApis as routerPrivateApis } from '@wordpress/router';
import { store as blockEditorStore } from '@wordpress/block-editor';

/**
* Internal dependencies
@@ -248,9 +249,14 @@ export default function useInitEditedEntityFromURL() {
useResolveEditedEntityAndContext( params );

const { setEditedEntity } = useDispatch( editSiteStore );
const { __unstableSetEditorMode, resetZoomLevel } = unlock(
useDispatch( blockEditorStore )
);

useEffect( () => {
if ( isReady ) {
__unstableSetEditorMode( 'edit' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we set an initial state instead? This is triggering a user preference to be set, which in turn triggers a REST API request to store this preference in the database, just by loading the site editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's not good. Let's try and find a better fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah...this doesn't affect WP 6.7 in the same way because it doesn't store the mode in a preference (only in state).

Nonetheless, this seems suboptimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm right we no longer want or need to set the editor mode here in trunk. If the mode is persisted as a preference then there's no need to reset it here. All we want to do is reset the zoom level.

In WP 6.7 the editor mode is relevant (and is only stored in state) so we can leave "as is".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Candidate fix in #66452.

resetZoomLevel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed? It feels like a very weird place for this action to be called (sync hook)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the requirement to reset the zoom level when you move to a new entity has changed.

I struggled to find a suitable location for this. I assume the concern is that we're calling a UI-specific hook (i.e. zoom level) in a hook which is primarily concerned with initialisation of editor state?

Any more information on your concerns may help to refactor this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually removed this in a PR, so I could have introduced some regressions possibly. Based on your description, I would say we have two options:

  • Potentially add this call to the before calling onNavigateEntity or something. (maybe a bit fragile)
  • Potentially add this to EditorProvider when the "post" prop (id, type) change.

setEditedEntity( postType, postId, context );
}
}, [ isReady, postType, postId, context, setEditedEntity ] );