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

Zoom Out: Disable zooming out when Distraction Free mode is activated #67028

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Nov 15, 2024

Fixes: #67025

What?

The Zoom Out and Distraction Free Mode features currently have conflicting behaviors. When Distraction Free Mode is enabled, users can still zoom out and add patterns, which undermines the purpose of Distraction Free Mode.

This pull request addresses the issue by resolving these conflicting behaviors to maintain the intended functionality of Distraction Free Mode.

The following are the fixes made in this PR:

  1. If Distraction Free mode is already activated and the user tries to activate the zoomed-out state using the keyboard shortcut, this action will then disable the distraction-free mode.
  2. Zoom Out Toggle will be completely disabled in Distraction Free Mode.
  3. If the Zoom Out state is active and the user decides to enter Distraction Free mode either using the keyboard shortcut or from the options, then the zoom out state will reset and then the Distraction Free mode will be activated.

Why?

The PR ensures Distraction Free Mode functions as intended by preventing disruptive actions like zooming out and adding patterns, preserving focus, and enhancing user experience.

How?

  1. Updated the toggleDistractionFree() function to include resetting the zoom level.
  2. Disable the Zoom Out Toggle when Distraction Free mode is activated.
  3. Update zoom out's callback function to disable distraction-free mode when zoomed out.

Testing Instructions

  1. Go to editing a post.
  2. Turn on Distraction Free Mode.
  3. The Zoomed Out state Toggle should now not be rendered.
  4. If the page is already Zoomed Out, then the page again Zooms In gracefully when distraction-free mode is toggled.

Screenshots or screencast ( Updated )

zoom-out-new-720p.mov

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review November 15, 2024 11:29
Copy link

github-actions bot commented Nov 15, 2024

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: yogeshbhutkar <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: draganescu <[email protected]>

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

…rs and update Toolbar to hide zoom out under distraction-free
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.

Thank you, @yogeshbhutkar!

Users can enable distraction-free mode through the Options dropdown, Commands and Keyboard shortcuts. The fix in the MoreMenu component only covers one case.

I think it would be better to update the toggleDistractionFree store action. I pushed similar fix in #66262, you can use it as example.

P.S. Let's also update the PR title/description to better reflect recent changes.

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] Distraction Free A preference in the Post and Site Editor that limits distractions to focus the editing experience [Feature] Zoom Out labels Nov 15, 2024
@Mamaduka Mamaduka requested a review from t-hamano November 15, 2024 13:22
@t-hamano
Copy link
Contributor

Thanks for the PR!

Just like Distraction free mode, there is a shortcut for zoom out mode. When zoom out mode is enabled via the keyboard shortcut, it is better to disable Distraction free mode.

@yogeshbhutkar
Copy link
Contributor Author

Thank you, @yogeshbhutkar!

Users can enable distraction-free mode through the Options dropdown, Commands and Keyboard shortcuts. The fix in the MoreMenu component only covers one case.

I think it would be better to update the toggleDistractionFree store action. I pushed similar fix in #66262, you can use it as example.

P.S. Let's also update the PR title/description to better reflect recent changes.

Hi @Mamaduka, thanks for taking the time to suggest changes. I've refactored the action to reset the zoom when distraction-free mode is activated. This also enabled the changes to be seamlessly integrated with the keyboard shortcuts.

@yogeshbhutkar
Copy link
Contributor Author

Thanks for the PR!

Just like Distraction free mode, there is a shortcut for zoom out mode. When zoom out mode is enabled via the keyboard shortcut, it is better to disable Distraction free mode.

Hi @t-hamano, thanks for taking the time to suggest changes. I've incorporated the change where when the user uses the keyboard shortcut to enable zoom out state and if distraction mode is enabled, then this action will, in turn, disable the distraction-free mode and enable the zoomed-out state.

@yogeshbhutkar yogeshbhutkar changed the title Zoom Out: Enhance ZoomOutModeInserters to support distraction-free mode Zoom Out: Disable zooming out when Distraction Free mode is activated Nov 18, 2024
Copy link
Member

Choose a reason for hiding this comment

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

We can revert changes from this file; the zoom-out toggle will be disabled or hidden in distraction-free mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Mamaduka,

Thank you for taking the time to review the PR. The changes in this file address the feedback provided in this comment: WordPress/gutenberg#67028 (issuecomment-2478844725).

Regarding the zoom-out toggle, should it be made entirely inaccessible, including via keyboard shortcuts? Or, as per @t-hamano's comment, should the current implementation be retained, allowing keyboard shortcuts to zoom out, which would automatically disable distraction-free mode?

Looking forward to your guidance.

Copy link
Member

Choose a reason for hiding this comment

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

This component renders the UI element for the toggle; changing logic here doesn't change login for keyboard shortcuts. Since the button will be disabled or hidden, there's no need for logic updates.

Keyboard shortcut behavior can be handled in a separate PR.

Comment on lines 839 to 840
// Exit zoom out state when toggling distraction free mode.
unlock( registry.dispatch( blockEditorStore ) ).resetZoomLevel();
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this into the registry.batch block above.

P.S. The inline comment seems redundant here.

Comment on lines 173 to 175
<ZoomOutToggle
disabled={ forceDisableBlockTools || isDistractionFree }
/>
Copy link
Member

Choose a reason for hiding this comment

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

It looks like other settings buttons in the header are hidden via CSS. I wonder if we should do the same here.

cc @draganescu

& > .editor-header__toolbar .editor-document-tools__document-overview-toggle,
& > .editor-header__settings > .editor-preview-dropdown,
& > .editor-header__settings > .interface-pinned-items {
display: none;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Mamaduka,

Based on the comment above, if I use CSS to hide the button, a potential issue arises: pressing cmd + shift + 0 will still trigger the zoom-out action via the shortcut, as the logic resides in the callback function within the ZoomOutToggle component.

useShortcut( 'core/editor/zoom', () => {
        if ( isZoomOut ) {
	        resetZoomLevel();
        } else {
	        setZoomLevel( 'auto-scaled' );
        }
} );

To address this, I see two potential solutions:

  1. Update the callback function inside useShortcut to include a check for isDistractionFree.
  2. Avoid rendering the ZoomOutToggle component when isDistractionFree mode is enabled and not use CSS to hide.

I’d appreciate your input on which approach would be preferable.

Copy link
Member

Choose a reason for hiding this comment

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

The useShortcut also accepts options argument, and we can disable the shortcut when isDistractionFree is true.

Example:

useShortcut(
'core/editor/toggle-mode',
() => {
switchEditorMode(
getEditorMode() === 'visual' ? 'text' : 'visual'
);
},
{
isDisabled: isModeToggleDisabled,
}
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'd like the Zoom Out button to be hidden rather than disabled when the Distraction free mode is enabled. What do you think?

I don't know why the other buttons are hidden with CSS, but as for the Zoom Out toggle button, it looks fine to render it conditionally using useViewportMatch etc 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why the other buttons are hidden with CSS

I don't know this either; it could be related to animations. Let's follow the current example from the core for hiding similar buttons; if the hiding method is changed in the future, we can do that in one go.

@Mamaduka Mamaduka requested a review from draganescu November 19, 2024 07:18
@yogeshbhutkar
Copy link
Contributor Author

I've made the patches as per our discussion and the PR is now ready to be reviewed again. Thanks for all the suggestions @Mamaduka and @t-hamano.

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 the follow-ups, @yogeshbhutkar!

Everything works as expected ✅

@Mamaduka Mamaduka merged commit c772226 into WordPress:trunk Nov 19, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.8 milestone Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Distraction Free A preference in the Post and Site Editor that limits distractions to focus the editing experience [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.

Conflict between Zoom Out mode and Distraction free mode
3 participants