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

Make zoom out vertical toolbar consistent #65627

Merged
merged 23 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
598d612
enable vertical toolbar for non full width elements, anchor based on …
draganescu Sep 24, 2024
ced9cbd
Update packages/block-editor/src/components/block-popover/index.js
draganescu Sep 25, 2024
31f69eb
Update packages/block-editor/src/components/block-popover/index.js
draganescu Sep 25, 2024
0d91f44
make zoom out check a dependency of the memoization, improve code rea…
draganescu Sep 30, 2024
2bb5176
comment typos
draganescu Oct 1, 2024
bb2b790
subscribe to state instead of calculating zoom out view state when ca…
draganescu Oct 1, 2024
216f472
get the section wrapper for anchoring instead of the parent
draganescu Oct 2, 2024
64b48d6
use a selector instead of computing on the fly the parent section
draganescu Oct 2, 2024
457db03
check if the block element exists yet before computing the anchor
draganescu Oct 2, 2024
6c3aa91
check if the block element exists yet before computing the anchor
draganescu Oct 2, 2024
605e660
differentiate between section toolbar and block toolbar for correct p…
draganescu Oct 4, 2024
cc9ec16
address some nits
draganescu Oct 4, 2024
086fe16
make the select in anchor setting rerun when block selection changes
draganescu Oct 7, 2024
1fb06f5
fix bug with anchor rect when zoom out not engaged
draganescu Oct 7, 2024
7589c5d
fix typo
draganescu Oct 7, 2024
22286ac
Use root container element in post editor as popover anchor
getdave Oct 8, 2024
ee585e4
improve comment
draganescu Oct 9, 2024
97836d3
improve comment to max improvement possible
draganescu Oct 9, 2024
dfa04be
mega nit commit
draganescu Oct 9, 2024
b95d7ef
Fix bug with Posts with no full width blocks
getdave Oct 10, 2024
9fadd33
give up on section root, always seek canvas element to position verti…
draganescu Oct 14, 2024
0afb084
introduce the concept of canvas via a 1st variable
draganescu Oct 14, 2024
f5a632e
Use `__unstableContentRef` for zoomed out toolbar positioning instead…
talldan Oct 14, 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
66 changes: 64 additions & 2 deletions packages/block-editor/src/components/block-popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import clsx from 'clsx';
*/
import { useMergeRefs } from '@wordpress/compose';
import { Popover } from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import {
forwardRef,
useMemo,
Expand All @@ -21,6 +22,8 @@ import {
import { useBlockElement } from '../block-list/use-block-props/use-block-refs';
import usePopoverScroll from './use-popover-scroll';
import { rectUnion, getVisibleElementBounds } from '../../utils/dom';
import { store as blockEditorStore } from '../../store';
import { unlock } from '../../lock-unlock';

const MAX_POPOVER_RECOMPUTE_COUNTER = Number.MAX_SAFE_INTEGER;

Expand Down Expand Up @@ -74,12 +77,38 @@ function BlockPopover(
};
}, [ selectedElement ] );

const { isZoomOut, parentSectionBlock, isSectionSelected } = useSelect(
( select ) => {
const {
isZoomOut: isZoomOutSelector,
getSectionRootClientId,
getParentSectionBlock,
getBlockOrder,
} = unlock( select( blockEditorStore ) );

return {
isZoomOut: isZoomOutSelector(),
parentSectionBlock:
getParentSectionBlock( clientId ) ?? clientId,
isSectionSelected: getBlockOrder(
getSectionRootClientId()
).includes( clientId ),
};
},
[ clientId ]
);
draganescu marked this conversation as resolved.
Show resolved Hide resolved

// This element is used to position the zoom out view vertical toolbar
// correctly, relative to the selected section.
const parentSectionElement = useBlockElement( parentSectionBlock );

const popoverAnchor = useMemo( () => {
if (
// popoverDimensionsRecomputeCounter is by definition always equal or greater
// than 0. This check is only there to satisfy the correctness of the
// exhaustive-deps rule for the `useMemo` hook.
popoverDimensionsRecomputeCounter < 0 ||
( isZoomOut && ! parentSectionElement ) ||
! selectedElement ||
( bottomClientId && ! lastSelectedElement )
) {
Expand All @@ -88,6 +117,35 @@ function BlockPopover(

return {
getBoundingClientRect() {
// The zoom out view has a vertical block toolbar that should always
// be on the edge of the canvas, aligned to the top of the currently
// selected section. This condition changes the anchor of the toolbar
// to the section instead of the block to handle blocks that are
// not full width and nested blocks to keep section height.
if ( isZoomOut && isSectionSelected ) {
// Compute the height based on the parent section of the
// selected block, because the selected block may be
// shorter than the section.
const canvasElementRect = getVisibleElementBounds(
__unstableContentRef.current
);
const parentSectionElementRect =
getVisibleElementBounds( parentSectionElement );
const anchorHeight =
parentSectionElementRect.bottom -
parentSectionElementRect.top;

// Always use the width of the section root element to make sure
// the toolbar is always on the edge of the canvas.
const anchorWidth = canvasElementRect.width;
return new window.DOMRectReadOnly(
canvasElementRect.left,
parentSectionElementRect.top,
anchorWidth,
anchorHeight
);
}

return lastSelectedElement
? rectUnion(
getVisibleElementBounds( selectedElement ),
Expand All @@ -98,10 +156,14 @@ function BlockPopover(
contextElement: selectedElement,
};
}, [
popoverDimensionsRecomputeCounter,
isZoomOut,
parentSectionElement,
selectedElement,
bottomClientId,
lastSelectedElement,
selectedElement,
popoverDimensionsRecomputeCounter,
isSectionSelected,
__unstableContentRef,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the change here. Can you explain why the block popover component needs to be aware of zoom-out...

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I'm confused is because for me the "BlockPopover" is just a component that gives you the "position" of the block. So I don't see how zoom-out is related. It's not about giving the position of the toolbar, it's about giving the position of the block. It just happens that toolbar is adjacent to the block (in most cases). I think if we need to adapt the position differently in zoom-out, we should either have clear props in this component to change the behavior (for instance a prop that clearly indicates how we want to change the behavior) or do this computation elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"BlockPopover" is just a component that gives you the "position" of the block.

I don't know what to say about that, since this does render a whole popover, it's not some utility called to get the position. I do get the concern though.

I think if we need to adapt the position differently in zoom-out, we should either have clear props in this component to change the behavior

That is a good alternative sure. But at some point someone will have to "know" about zoom out.

Let's wait on the faith of this toolbar and if it sticks around return to reflect in code the idea that toolbars "just happen" to be adjacent to blocks. This was intended as a bug fix and it does not introduce a big glaring problem IMO just some minor tech debt reflecting the issue with multiple toolbar types.

Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes :)

] );

if ( ! selectedElement || ( bottomClientId && ! lastSelectedElement ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { isUnmodifiedDefaultBlock } from '@wordpress/blocks';
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import { unlock } from '../../lock-unlock';

/**
* Source of truth for which block tools are showing in the block editor.
Expand All @@ -24,7 +25,9 @@ export function useShowBlockTools() {
getSettings,
__unstableGetEditorMode,
isTyping,
} = select( blockEditorStore );
getBlockOrder,
getSectionRootClientId,
} = unlock( select( blockEditorStore ) );

const clientId =
getSelectedBlockClientId() || getFirstMultiSelectedBlockClientId();
Expand All @@ -42,10 +45,14 @@ export function useShowBlockTools() {
editorMode === 'edit' &&
isEmptyDefaultBlock;
const isZoomOut = editorMode === 'zoom-out';
const isSectionSelected = getBlockOrder(
getSectionRootClientId()
).includes( clientId );
const _showZoomOutToolbar =
clientId &&
isZoomOut &&
block?.attributes?.align === 'full' &&
! _showEmptyBlockSideInserter;
! _showEmptyBlockSideInserter &&
isSectionSelected;
const _showBlockToolbarPopover =
! _showZoomOutToolbar &&
! getSettings().hasFixedToolbar &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import clsx from 'clsx';
/**
* Internal dependencies
*/
import BlockPopover from '../block-popover';
import { PrivateBlockPopover as BlockPopover } from '../block-popover';
import useBlockToolbarPopoverProps from './use-block-toolbar-popover-props';
import useSelectedBlockToolProps from './use-selected-block-tool-props';
import ZoomOutToolbar from './zoom-out-toolbar';
Expand All @@ -29,6 +29,7 @@ export default function ZoomOutPopover( { clientId, __unstableContentRef } ) {

return (
<BlockPopover
__unstableContentRef={ __unstableContentRef }
clientId={ capturingClientId || clientId }
bottomClientId={ lastClientId }
className={ clsx( 'zoom-out-toolbar-popover', {
Expand Down
Loading