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

Position: Refactor "Position" controls panel to use ToolsPanel instead of PanelBody #67967

Merged
merged 8 commits into from
Jan 21, 2025
Merged

Position: Refactor "Position" controls panel to use ToolsPanel instead of PanelBody #67967

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6b82f9f
Refactor "Position" controls panel to use `ToolsPanel` instead of `Pa…
yogeshbhutkar Dec 13, 2024
d41f5f9
Refactor imports
yogeshbhutkar Dec 13, 2024
6aa96a2
Refactor `position-controls-panel` to reset multiple selected blocks
yogeshbhutkar Jan 20, 2025
fbee655
Refactor `PositionControlsPanel` to simplify block attribute retrieval
yogeshbhutkar Jan 20, 2025
ccd5773
Refactor: Simplify the logic
yogeshbhutkar Jan 20, 2025
5fce618
Merge branch 'trunk' into 67950-refactor-position-block
yogeshbhutkar Jan 21, 2025
cf9871e
Refactor `PositionControlsPanel` to improve block attribute handling …
yogeshbhutkar Jan 21, 2025
5b17dea
Refactor `PositionControlsPanel` to clean up empty style properties f…
yogeshbhutkar Jan 21, 2025
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
71 changes: 43 additions & 28 deletions packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
* WordPress dependencies
*/
import {
PanelBody,
__experimentalUseSlotFills as useSlotFills,
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { useLayoutEffect, useState } from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

/**
Expand All @@ -17,38 +17,53 @@ import { default as InspectorControls } from '../inspector-controls';
import { store as blockEditorStore } from '../../store';

const PositionControlsPanel = () => {
const [ initialOpen, setInitialOpen ] = useState();
const { selectedClientIDs, hasPositionAttribute } = useSelect(
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
( select ) => {
const { getBlocksByClientId, getSelectedBlockClientIds } =
select( blockEditorStore );

// Determine whether the panel should be expanded.
const { multiSelectedBlocks } = useSelect( ( select ) => {
const { getBlocksByClientId, getSelectedBlockClientIds } =
select( blockEditorStore );
const clientIds = getSelectedBlockClientIds();
return {
multiSelectedBlocks: getBlocksByClientId( clientIds ),
};
}, [] );
const selectedBlockClientIds = getSelectedBlockClientIds();
const selectedBlocks = getBlocksByClientId(
selectedBlockClientIds
);

useLayoutEffect( () => {
// If any selected block has a position set, open the panel by default.
// The first block's value will still be used within the control though.
if ( initialOpen === undefined ) {
setInitialOpen(
multiSelectedBlocks.some(
return {
selectedClientIDs: selectedBlockClientIds,
hasPositionAttribute: selectedBlocks?.some(
( { attributes } ) => !! attributes?.style?.position?.type
)
);
}
}, [ initialOpen, multiSelectedBlocks, setInitialOpen ] );
),
};
},
[]
);

const { updateBlockAttributes } = useDispatch( blockEditorStore );

function resetPosition() {
updateBlockAttributes( selectedClientIDs, {
style: {
position: {
type: undefined,
},
},
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

If a block has other styles, those styles will be reset unintentionally as well. I think all styles that are not related to position should be preserved. See this example.

Copy link
Contributor Author

@yogeshbhutkar yogeshbhutkar Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In the latest commit, I used the uniqueByBlock attribute and passed keyed clientIds with their attributes as the second parameter in updateBlockAttributes. This ensures the styles of the respective blocks are preserved when multiple blocks are reset.

However, I found this bug (within the existing implementation, not caused due to this PR):

Screen.Recording.2025-01-21.at.12.28.53.PM.mov

Setting the position to sticky when multiple blocks are selected applies the styles of the first block to all the other selected blocks overriding their applied styles.

I believe we should also update the onChangeType here:

gutenberg/packages/block-editor/src/hooks/position.js

Line 239 in 39ed61d

const onChangeType = ( next ) => {

Updating it for all the selected blocks fixed the bug for me. Do I create a separate PR as this is a separate bug?

const onChangeType = ( next ) => {
  const placementValue = '0px';
  const attributesByClientId = Object.fromEntries(
	  selectedBlocks?.map(
		  ( { clientId: blockClientId, attributes } ) => [
			  blockClientId,
			  {
				  style: cleanEmptyObject( {
					  ...attributes?.style,
					  position: {
						  ...attributes?.style?.position,
						  type: next,
						  top:
							  next === 'sticky' || next === 'fixed'
								  ? placementValue
								  : undefined,
					  },
				  } ),
			  },
		  ]
	  )
  );
  updateBlockAttributes( selectedClientIds, attributesByClientId, true );
};

Also, there's already a resetPosition function exported here:

gutenberg/packages/block-editor/src/hooks/position.js

Line 153 in 39ed61d

export function resetPosition( { attributes = {}, setAttributes } ) {

But, it accepts setAttributes, considering the above bug, I believe it's better to remove this implementation as it's not used anywhere in favor of the resetPosition in this PR.

CC: @t-hamano

}

return (
<PanelBody
<ToolsPanel
className="block-editor-block-inspector__position"
title={ __( 'Position' ) }
initialOpen={ initialOpen ?? false }
label={ __( 'Position' ) }
resetAll={ resetPosition }
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
>
<InspectorControls.Slot group="position" />
</PanelBody>
<ToolsPanelItem
isShownByDefault={ hasPositionAttribute }
label={ __( 'Position' ) }
hasValue={ () => !! hasPositionAttribute }
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
onDeselect={ resetPosition }
>
<InspectorControls.Slot group="position" />
</ToolsPanelItem>
</ToolsPanel>
);
};

Expand Down
Loading