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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d69ae8f
Zoom Out: Enhance ZoomOutModeInserters to support distraction-free mode
yogeshbhutkar Nov 15, 2024
51ea6fd
Refactor: Remove distraction-free mode checks from ZoomOutModeInserte…
yogeshbhutkar Nov 15, 2024
ab28e26
Refactor: Update toggleDistractionFree to include resetting zoom out …
yogeshbhutkar Nov 18, 2024
b037bf4
Refactor: Update ZoomOutToggle to disable in distraction-free mode an…
yogeshbhutkar Nov 18, 2024
bcd29c4
Refactor: Use CSS to hide the ZoomOutToggle
yogeshbhutkar Nov 19, 2024
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
6 changes: 5 additions & 1 deletion packages/editor/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ function Header( {
showIconLabels,
hasFixedToolbar,
hasBlockSelection,
isDistractionFree,
} = useSelect( ( select ) => {
const { get: getPreference } = select( preferencesStore );
const {
Expand All @@ -80,6 +81,7 @@ function Header( {
hasFixedToolbar: getPreference( 'core', 'fixedToolbar' ),
hasBlockSelection:
!! select( blockEditorStore ).getBlockSelectionStart(),
isDistractionFree: getPreference( 'core', 'distractionFree' ),
};
}, [] );

Expand Down Expand Up @@ -168,7 +170,9 @@ function Header( {
/>

{ canBeZoomedOut && isWideViewport && (
<ZoomOutToggle disabled={ forceDisableBlockTools } />
<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

gutenberg/packages/editor/src/components/header/style.scss

Lines 257 to 261 in 564f069

& > .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:

gutenberg/packages/editor/src/components/global-keyboard-shortcuts/index.js

Lines 46 to 56 in 564f069

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.

) }

{ ( isWideViewport || ! showIconLabels ) && (
Expand Down
3 changes: 3 additions & 0 deletions packages/editor/src/components/zoom-out-toggle/index.js
View file
Open in desktop
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.

Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const ZoomOutToggle = ( { disabled } ) => {
),
} ) );

const { set: setPreference } = useDispatch( preferencesStore );

const { resetZoomLevel, setZoomLevel } = unlock(
useDispatch( blockEditorStore )
);
Expand Down Expand Up @@ -57,6 +59,7 @@ const ZoomOutToggle = ( { disabled } ) => {
resetZoomLevel();
} else {
setZoomLevel( 'auto-scaled' );
setPreference( 'core', 'distractionFree', false );
}
} );

Expand Down
3 changes: 3 additions & 0 deletions packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,9 @@ export const toggleDistractionFree =
dispatch.setIsInserterOpened( false );
dispatch.setIsListViewOpened( false );
} );

// 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.

}
registry.batch( () => {
registry
Expand Down
Loading