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

Reset zoom level on component unmount #69087

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

Infinite-Null
Copy link
Contributor

What?

Closes #69086

Reset zoom level when editor component unmounts to prevent stale zoom states.

Why?

The zoom state and button remained active after responsive changes, causing inconsistency between the visual zoom effect and the button state. This led to a confusing user experience in which the zoom button appeared pressed, but no zoom effect was visible.

How?

Added a cleanup effect in the ZoomOutToggle component that calls resetZoomLevel when the component unmounts. This ensures the zoom state is properly reset between editor sessions and responsive changes.

Testing Instructions

  1. Open the WordPress block editor
  2. Click the zoom-out button
  3. Resize browser window to a smaller width
  4. Resize browser window back to larger width
  5. Verify the zoom state and button are in sync

Screencast

Screen.Recording.2025-02-07.at.12.27.40.PM.mov

Copy link

github-actions bot commented Feb 7, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Infinite-Null <[email protected]>
Co-authored-by: Mamaduka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] Zoom Out labels Feb 7, 2025
@Mamaduka
Copy link
Member

Mamaduka commented Feb 7, 2025

Do you know what part of the code disables zoomout mode for smaller screens?

Controlling a global state-based component's mounted state is tricky and usually causes more bugs. This bug probably has a different root cause; I suggest finding it and fixing the issue there.

@Infinite-Null
Copy link
Contributor Author

Sure @Mamaduka, I’ll check what disables zoom-out on smaller screens and find the root cause.

@Infinite-Null
Copy link
Contributor Author

Infinite-Null commented Feb 12, 2025

Hi @Mamaduka,
I was able to find the root cause and updated my PR. While doing research I found an issue:

The useLayoutEffect hook is triggered when the viewport width is at or above 782px. However, the condition:

const isMediumOrBigger = window.matchMedia('(min-width: 782px)').matches;

does not return false when the viewport width gradually decreases to exactly 782px. This is because the (min-width: 782px) media query does not include 782px itself it applies only to widths greater than 782px.

As a result, when resizing the viewport slowly, the condition remains true even at 782px. However, when the viewport is resized quickly, bypassing the 782px threshold, the condition updates correctly and evaluates to false as expected.

A fix to this would be to change code like:

const isMediumOrBigger = window.matchMedia('(min-width: 783px)').matches;

Or

const isMediumOrBigger = window.width>=782;

Screencast:

Screen.Recording.2025-02-12.at.10.53.59.AM.mov

Should I open an issue about this also I am quite not sure which solution would be better.
I would love to hear your thoughts.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this, @Infinite-Null!

I left a a couple of of notes, but unfortunately, I don't have time to test the proposed solution properly.

Pinging @WordPress/gutenberg-core in case someone has time to have a look.

@@ -17,6 +22,8 @@ export function useAdaptEditorToCanvas( canvas ) {
} = useDispatch( editorStore );
const { get: getPreference } = useSelect( preferencesStore );
const registry = useRegistry();
const { resetZoomLevel } = unlock( useDispatch( blockEditorStore ) );
Copy link
Member

Choose a reason for hiding this comment

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

The useDispatch( blockEditorStore ) is also defined above. Let's move and combine them in single call.

@@ -17,6 +22,8 @@ export function useAdaptEditorToCanvas( canvas ) {
} = useDispatch( editorStore );
const { get: getPreference } = useSelect( preferencesStore );
const registry = useRegistry();
const { resetZoomLevel } = unlock( useDispatch( blockEditorStore ) );
const { isZoomOut } = unlock( select( blockEditorStore ) );
Copy link
Member

Choose a reason for hiding this comment

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

The components shouldn't use global select it doesn't react to store changes.

Suggested change
const { isZoomOut } = unlock( select( blockEditorStore ) );
const { isZoomOut } = unlock( useSelect( blockEditorStore ) );

@Infinite-Null
Copy link
Contributor Author

Hi @Mamaduka,
I have made the necessary changes please take a look at it at your convenience. Thank You!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Zoom Out [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoom state and button remain active after responsive changes
2 participants