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

yogeshbhutkar · 2024-12-13T10:29:14Z

Fixes: #67950

What?

This PR introduces the usage of ToolsPanel instead of PanelBody inside the Position block support UI.

Why?

This PR is a part of #67813 which standardizes the usage of ToolsPanel inside Block Support UI.

How?

PanelBody is replaced with the corresponding ToolsPanel component.

Testing Instructions

Insert a Group Block.
Notice the presence of ToolsPanel component.
Try to reset the updated values to test the reset functions.

Screencast

Screen.Recording.2024-12-13.at.4.03.43.PM.mov

…nelBody`

github-actions · 2024-12-13T10:48:44Z

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

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

fabiankaegy

Before this update the position control also allowed you to update its value when multiple group blocks (blocks that support position) were selected. That code has been removed here.

For a single block this already works well. But we need to also handle multi block selection again :)

yogeshbhutkar · 2024-12-16T04:13:15Z

Before this update the position control also allowed you to update its value when multiple group blocks (blocks that support position) were selected. That code has been removed here.

For a single block this already works well. But we need to also handle multi block selection again :)

Before this update ( Trunk Branch ):

After this update ( The current PR ):

As demonstrated in the GIFs above, the position control indeed is capable of updating values when multiple group blocks are selected. It's also found to be working with 2 Group Blocks and 1 Row Block together.

Analysis of removed code:

const [ initialOpen, setInitialOpen ] = useState();

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

The above block is responsible to return the blocks that are currently selected.

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(
			  ( { attributes } ) => !! attributes?.style?.position?.type
		  )
	  );
  }
}, [ initialOpen, multiSelectedBlocks, setInitialOpen ] );

This block of code toggles the state initialOpen if some of the blocks that are selected have position?.type. i.e., were assigned a position before.

This toggled state is used as an initial value for the initialOpen prop in the previous PanelBody component.

<PanelBody
	className="block-editor-block-inspector__position"
	title={ __( 'Position' ) }
	initialOpen={ initialOpen ?? false }
>
	<InspectorControls.Slot group="position" />
</PanelBody>

But, we don't need the initialOpen prop, hence, the above code is not required when using the ToolsPanel component, and hence was removed. As per my understanding, it has nothing to do with being able to update multiple selected blocks.

Please let me know if I'm missing out something in my approach, but as per the comment, @fabiankaegy mentioned the component now does not support updating position when multiple blocks are selected, but that doesn't seem to be the case and multi-block selection + updation, in my opinion is working properly as demonstrated in the 2nd GIF.

Can you please provide a screencast for my reference, if this doesn't answer the bug that you've mentioned?

CC: @fabiankaegy

yogeshbhutkar · 2024-12-18T08:22:05Z

Hi @fabiankaegy, circling back to check if the above comment resolves this review. Please let me know if any changes still need to be made.

yogeshbhutkar · 2024-12-30T08:52:33Z

Hi @Mamaduka,
Could you please review this PR when you get a moment? This PR wasn't getting enough momentum and the issue stands unresolved.

t-hamano

@yogeshbhutkar Thanks for the PR!

But, we don't need the initialOpen prop, hence, the above code is not required when using the ToolsPanel component, and hence was removed. As per my understanding, it has nothing to do with being able to update multiple selected blocks.

It is true that there is no initialOpen prop for ToolsPanel or ToolsPanelItem, but an equivalent prop might be the isShownByDefault prop of the ToolsPanelItem component.

For example, is it possible to use the isShowbByDefault prop conditionally, as shown below?

const PositionControlsPanel = () => {
	// ...

	// If any selected block has a position set, show the control by default.
	const isShownByDefault = useSelect( ( select ) => {
		const { getBlocksByClientId, getSelectedBlockClientIds } =
			select( blockEditorStore );
		const clientIds = getSelectedBlockClientIds();
		const multiSelectedBlocks = getBlocksByClientId( clientIds );
		return multiSelectedBlocks.some(
			( { attributes } ) => !! attributes?.style?.position?.type
		);
	}, [] );

	// ...

	return (
		<ToolsPanel>
			<ToolsPanelItem
				isShownByDefault={ isShownByDefault }
				label={ __( 'Position' ) }
			></ToolsPanelItem>
		</ToolsPanel>
	);
};

packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js

yogeshbhutkar · 2025-01-20T07:02:14Z

Thanks for the review @t-hamano, I've refactored the PR to accommodate the suggested changes.

Screen.Recording.2025-01-20.at.12.21.08.PM.mov

t-hamano · 2025-01-21T03:01:55Z

packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js

+		updateBlockAttributes( selectedClientIDs, {
+			style: {
+				position: {
+					type: undefined,
+				},
+			},
+		} );


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.

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

packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js

…and reset logic

…or selected blocks

t-hamano

LGTM!

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

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.

Good catch! This issue could be addressed in a follow-up.

I believe the controls work correctly when multiple blocks are selected.

yogeshbhutkar added 2 commits December 13, 2024 15:58

Refactor "Position" controls panel to use ToolsPanel instead of `Pa…

6b82f9f

…nelBody`

Refactor imports

d41f5f9

yogeshbhutkar marked this pull request as ready for review December 13, 2024 10:48

yogeshbhutkar requested a review from ellatrix as a code owner December 13, 2024 10:48

yogeshbhutkar changed the title ~~Refactor "Position" controls panel to use ToolsPanel instead of PanelBody~~ Position: Refactor "Position" controls panel to use ToolsPanel instead of PanelBody Dec 13, 2024

fabiankaegy added [Type] Enhancement A suggestion for improvement. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Dec 13, 2024

fabiankaegy previously requested changes Dec 13, 2024

View reviewed changes

yogeshbhutkar requested a review from fabiankaegy December 16, 2024 04:16

t-hamano self-requested a review January 18, 2025 04:45

t-hamano reviewed Jan 18, 2025

View reviewed changes

packages/block-editor/src/components/inspector-controls-tabs/position-controls-panel.js Outdated Show resolved Hide resolved

yogeshbhutkar added 2 commits January 20, 2025 11:55

Refactor position-controls-panel to reset multiple selected blocks

6aa96a2

Refactor PositionControlsPanel to simplify block attribute retrieval

fbee655

Refactor: Simplify the logic

ccd5773

yogeshbhutkar requested a review from t-hamano January 20, 2025 08:14

t-hamano reviewed Jan 21, 2025

View reviewed changes

yogeshbhutkar added 3 commits January 21, 2025 09:33

Merge branch 'trunk' into 67950-refactor-position-block

5fce618

Refactor PositionControlsPanel to improve block attribute handling …

cf9871e

…and reset logic

Refactor PositionControlsPanel to clean up empty style properties f…

5b17dea

…or selected blocks

t-hamano approved these changes Jan 21, 2025

View reviewed changes

t-hamano merged commit 34f09ca into WordPress:trunk Jan 21, 2025
64 of 65 checks passed

github-actions bot added this to the Gutenberg 20.2 milestone Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yogeshbhutkar commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

fabiankaegy left a comment

yogeshbhutkar commented Dec 16, 2024 •

edited

Loading

yogeshbhutkar commented Dec 18, 2024 •

edited

Loading

yogeshbhutkar commented Dec 30, 2024

t-hamano left a comment

yogeshbhutkar commented Jan 20, 2025 •

edited

Loading

t-hamano Jan 21, 2025

yogeshbhutkar Jan 21, 2025 •

edited

Loading

t-hamano left a comment

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

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

Conversation

yogeshbhutkar commented Dec 13, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Screencast

github-actions bot commented Dec 13, 2024 • edited Loading

fabiankaegy left a comment

Choose a reason for hiding this comment

yogeshbhutkar commented Dec 16, 2024 • edited Loading

Before this update ( Trunk Branch ):

After this update ( The current PR ):

Analysis of removed code:

yogeshbhutkar commented Dec 18, 2024 • edited Loading

yogeshbhutkar commented Dec 30, 2024

t-hamano left a comment

Choose a reason for hiding this comment

yogeshbhutkar commented Jan 20, 2025 • edited Loading

t-hamano Jan 21, 2025

Choose a reason for hiding this comment

yogeshbhutkar Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

t-hamano left a comment

Choose a reason for hiding this comment

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

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

yogeshbhutkar commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024 •

edited

Loading

yogeshbhutkar commented Dec 16, 2024 •

edited

Loading

yogeshbhutkar commented Dec 18, 2024 •

edited

Loading

yogeshbhutkar commented Jan 20, 2025 •

edited

Loading

yogeshbhutkar Jan 21, 2025 •

edited

Loading