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

Simplify zoom out insertion UX #65592

Closed
wants to merge 2 commits into from
+16 −184
Closed

Simplify zoom out insertion UX #65592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16d52c1
remove zoom out inserters, edit behavior of zoom out toggle and separ…
draganescu Sep 24, 2024
fa43644
add expand on hover back
draganescu Sep 24, 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
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ function Items( {
<ZoomOutSeparator
clientId={ clientId }
rootClientId={ rootClientId }
position="top"
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
/>
) }
<BlockListBlock
Expand All @@ -244,7 +243,6 @@ function Items( {
<ZoomOutSeparator
clientId={ clientId }
rootClientId={ rootClientId }
position="bottom"
/>
) }
</AsyncModeProvider>
Expand Down
25 changes: 8 additions & 17 deletions packages/block-editor/src/components/block-list/zoom-out-separator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,12 @@ import { useState } from '@wordpress/element';
import { store as blockEditorStore } from '../../store';
import { unlock } from '../../lock-unlock';

export function ZoomOutSeparator( {
clientId,
rootClientId = '',
position = 'top',
} ) {
export function ZoomOutSeparator( { clientId, rootClientId = '' } ) {
const [ isDraggedOver, setIsDraggedOver ] = useState( false );
const {
sectionRootClientId,
sectionClientIds,
isBlockSelected,
blockInsertionPoint,
blockInsertionPointVisible,
} = useSelect( ( select ) => {
Expand All @@ -37,11 +34,13 @@ export function ZoomOutSeparator( {
getBlockOrder,
isBlockInsertionPointVisible,
getSectionRootClientId,
isBlockSelected: _isBlockSelected,
} = unlock( select( blockEditorStore ) );

const root = getSectionRootClientId();
const sectionRootClientIds = getBlockOrder( root );
return {
isBlockSelected: _isBlockSelected,
sectionRootClientId: root,
sectionClientIds: sectionRootClientIds,
blockOrder: getBlockOrder( root ),
Expand All @@ -67,18 +66,10 @@ export function ZoomOutSeparator( {
return null;
}

if ( position === 'top' ) {
isVisible =
blockInsertionPointVisible &&
blockInsertionPoint.index === 0 &&
clientId === sectionClientIds[ blockInsertionPoint.index ];
}

if ( position === 'bottom' ) {
isVisible =
blockInsertionPointVisible &&
clientId === sectionClientIds[ blockInsertionPoint.index - 1 ];
}
isVisible =
isBlockSelected( clientId ) ||
( blockInsertionPointVisible &&
clientId === sectionClientIds[ blockInsertionPoint.index ] );

return (
<AnimatePresence>
Expand Down
6 changes: 0 additions & 6 deletions packages/block-editor/src/components/block-tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import BlockToolbarPopover from './block-toolbar-popover';
import ZoomOutPopover from './zoom-out-popover';
import { store as blockEditorStore } from '../../store';
import usePopoverScroll from '../block-popover/use-popover-scroll';
import ZoomOutModeInserters from './zoom-out-mode-inserters';
import { useShowBlockTools } from './use-show-block-tools';
import { unlock } from '../../lock-unlock';
import getEditorRegion from '../../utils/get-editor-region';
Expand Down Expand Up @@ -241,11 +240,6 @@ export default function BlockTools( {
name="__unstable-block-tools-after"
ref={ blockToolbarAfterRef }
/>
{ isZoomOutMode && (
<ZoomOutModeInserters
Copy link
Contributor

Choose a reason for hiding this comment

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

So this PR removes the always visible in-between inserters for zoom-out. Right? I think that's ok but should we restore "in-between" inserter in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's a matter of removing the isZoomOut here

gutenberg/packages/block-editor/src/components/block-tools/insertion-point.js

Line 152 in b681b50

if ( isZoomOutMode && operation !== 'insert' ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try this and see what happens.

__unstableContentRef={ __unstableContentRef }
/>
) }
</InsertionPointOpenRef.Provider>
</div>
);
Expand Down
47 changes: 0 additions & 47 deletions packages/block-editor/src/components/block-tools/zoom-out-mode-inserter-button.js
View file
Open in desktop

This file was deleted.

112 changes: 0 additions & 112 deletions packages/block-editor/src/components/block-tools/zoom-out-mode-inserters.js
View file
Open in desktop

This file was deleted.

8 changes: 8 additions & 0 deletions packages/editor/src/components/zoom-out-toggle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { store as preferencesStore } from '@wordpress/preferences';
/**
* Internal dependencies
*/
import { store as editorStore } from '../../store';
import { unlock } from '../../lock-unlock';

const ZoomOutToggle = () => {
Expand All @@ -27,11 +28,18 @@ const ZoomOutToggle = () => {
useDispatch( blockEditorStore )
);

const { setIsInserterOpened } = unlock( useDispatch( editorStore ) );

const handleZoomOut = () => {
if ( isZoomOut ) {
setIsInserterOpened( false );
resetZoomLevel();
} else {
setZoomLevel( 50 );
setIsInserterOpened( {
tab: 'patterns',
category: 'all',
Copy link
Contributor

Choose a reason for hiding this comment

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

In my test, if I "toggle zoom-out" mode, the inserter opens with the "all" category selected.

But If stay in zoom-out and close and then reopen the inserter, I'm not sure I understand why it shouldn't have the same effect (open "all" category).

The other question is where is the code responsible of enforcing "patterns" tab by default if I toggle the inserter in zoom-out? I think that code should do the same for "navigation" mode because otherwise, if we switch the "mode" when zoom-out is enabled, we kind of fallback to "blocks" by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But If stay in zoom-out and close and then reopen the inserter, I'm not sure I understand why it shouldn't have the same effect (open "all" category).

I think I subscribe to the intention behind the above, I am unsure if having the inserter button doing different things depending on the view is OK or confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I subscribe to the intention behind the above, I am unsure if having the inserter button doing different things depending on the view is OK or confusing.

I'm also not sure 100% but I think maybe we should give it a shot separately and see.

What about my second question, do you have any idea?

} );
}
__unstableSetEditorMode( isZoomOut ? 'edit' : 'zoom-out' );
};
Expand Down
Loading