Skip to content

useAvailableAlignments: avoid useSelect subscription when alignments array is empty #55449

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

Merged
jsnajdr merged 1 commit into from
Oct 19, 2023
Merged

useAvailableAlignments: avoid useSelect subscription when alignments array is empty #55449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 49 additions & 34 deletions packages/block-editor/src/components/block-alignment-control/use-available-alignments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,43 @@ export default function useAvailableAlignments( controls = DEFAULT_CONTROLS ) {
if ( ! controls.includes( 'none' ) ) {
controls = [ 'none', ...controls ];
}
const {
wideControlsEnabled = false,
themeSupportsLayout,
isBlockBasedTheme,
} = useSelect( ( select ) => {
const { getSettings } = select( blockEditorStore );
const settings = getSettings();
return {
wideControlsEnabled: settings.alignWide,
themeSupportsLayout: settings.supportsLayout,
isBlockBasedTheme: settings.__unstableIsBlockBasedTheme,
};
}, [] );
const isNoneOnly = controls.length === 1 && controls[ 0 ] === 'none';

const [ wideControlsEnabled, themeSupportsLayout, isBlockBasedTheme ] =
useSelect(
( select ) => {
// If `isNoneOnly` is true, we'll be returning early because there is
// nothing to filter on an empty array. We won't need the info from
// the `useSelect` but we must call it anyway because Rules of Hooks.
// So the callback returns early to avoid block editor subscription.
if ( isNoneOnly ) {
return [ false, false, false ];
}
Comment on lines +27 to +33

@Mamaduka Mamaduka Oct 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hopefully, the comment will mitigate this odd return value 😄

@jsnajdr jsnajdr Oct 19, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it bothers me that it looks so weird 🙂

On the other hand, this patch demonstrates a useSelect usage pattern that I think its quite neat and we could adopt it more widely: returning arrays instead of objects!

When writing:

const { themeSupportsLayout } = useSelect( ( select ) => ( { themeSupportsLayout: ... } ) );

the themeSupportsLayout identifier is repeated twice, and constructing and destructuring the object is a bit unwieldy. Compare this to an array:

const [ themeSupportsLayout ] = useSelect( ( select ) => ( [ ... ] ) );

That's much shorter and yet you don't lose any information. We're replacing the themeSupportsLayout with a 0 array index, basically. The field name doesn't matter because it's immediately destructured anyway.


const settings = select( blockEditorStore ).getSettings();
return [
settings.alignWide ?? false,
settings.supportsLayout,
settings.__unstableIsBlockBasedTheme,
];
},
[ isNoneOnly ]
);
const layout = useLayout();

if ( isNoneOnly ) {
return EMPTY_ARRAY;
}

const layoutType = getLayoutType( layout?.type );
const layoutAlignments = layoutType.getAlignments(
layout,
isBlockBasedTheme
);

if ( themeSupportsLayout ) {
const alignments = layoutAlignments.filter(
( { name: alignmentName } ) => controls.includes( alignmentName )
const layoutAlignments = layoutType.getAlignments(
layout,
isBlockBasedTheme
);
const alignments = layoutAlignments.filter( ( alignment ) =>
controls.includes( alignment.name )
);
// While we treat `none` as an alignment, we shouldn't return it if no
// other alignments exist.
Expand All @@ -55,25 +69,26 @@ export default function useAvailableAlignments( controls = DEFAULT_CONTROLS ) {
if ( layoutType.name !== 'default' && layoutType.name !== 'constrained' ) {
return EMPTY_ARRAY;
}
const { alignments: availableAlignments = DEFAULT_CONTROLS } = layout;
const enabledControls = controls
.filter(
( control ) =>
( layout.alignments || // Ignore the global wideAlignment check if the layout explicitely defines alignments.
wideControlsEnabled ||
! WIDE_CONTROLS.includes( control ) ) &&
availableAlignments.includes( control )
)
.map( ( enabledControl ) => ( { name: enabledControl } ) );

const alignments = controls
.filter( ( control ) => {
if ( layout.alignments ) {
return layout.alignments.includes( control );
}

if ( ! wideControlsEnabled && WIDE_CONTROLS.includes( control ) ) {
return false;
}

return DEFAULT_CONTROLS.includes( control );
} )
.map( ( name ) => ( { name } ) );

// While we treat `none` as an alignment, we shouldn't return it if no
// other alignments exist.
if (
enabledControls.length === 1 &&
enabledControls[ 0 ].name === 'none'
) {
if ( alignments.length === 1 && alignments[ 0 ].name === 'none' ) {
return EMPTY_ARRAY;
}

return enabledControls;
return alignments;
}
62 changes: 32 additions & 30 deletions packages/block-editor/src/hooks/align.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,20 +120,17 @@ export const withToolbarControls = createHigherOrderComponent(
( BlockEdit ) => ( props ) => {
const blockEdit = <BlockEdit key="edit" { ...props } />;
const { name: blockName } = props;
// Compute the block valid alignments by taking into account,
// if the theme supports wide alignments or not and the layout's
// availble alignments. We do that for conditionally rendering
// Slot.

const blockEditingMode = useBlockEditingMode();
if ( blockEditingMode !== 'default' ) {
return blockEdit;
}

const blockAllowedAlignments = getValidAlignments(
getBlockSupport( blockName, 'align' ),
hasBlockSupport( blockName, 'alignWide', true )
);

const validAlignments = useAvailableAlignments(
blockAllowedAlignments
).map( ( { name } ) => name );
const blockEditingMode = useBlockEditingMode();
if ( ! validAlignments.length || blockEditingMode !== 'default' ) {
if ( blockAllowedAlignments.length === 0 ) {
return blockEdit;
}

Expand All @@ -154,7 +151,7 @@ export const withToolbarControls = createHigherOrderComponent(
<BlockAlignmentControl
value={ props.attributes.align }
onChange={ updateAlignment }
controls={ validAlignments }
controls={ blockAllowedAlignments }
/>
</BlockControls>
{ blockEdit }
Expand All @@ -164,6 +161,23 @@ export const withToolbarControls = createHigherOrderComponent(
'withToolbarControls'
);

function BlockListBlockWithDataAlign( { block: BlockListBlock, props } ) {
const { name, attributes } = props;
const { align } = attributes;
const blockAllowedAlignments = getValidAlignments(
getBlockSupport( name, 'align' ),
hasBlockSupport( name, 'alignWide', true )
);
const validAlignments = useAvailableAlignments( blockAllowedAlignments );

let wrapperProps = props.wrapperProps;
if ( validAlignments.some( ( alignment ) => alignment.name === align ) ) {
wrapperProps = { ...wrapperProps, 'data-align': align };
}

return <BlockListBlock { ...props } wrapperProps={ wrapperProps } />;
}

/**
* Override the default block element to add alignment wrapper props.
*
Expand All @@ -173,30 +187,18 @@ export const withToolbarControls = createHigherOrderComponent(
*/
export const withDataAlign = createHigherOrderComponent(
( BlockListBlock ) => ( props ) => {
const { name, attributes } = props;
const { align } = attributes;
const blockAllowedAlignments = getValidAlignments(
getBlockSupport( name, 'align' ),
hasBlockSupport( name, 'alignWide', true )
);
const validAlignments = useAvailableAlignments(
blockAllowedAlignments
);

// If an alignment is not assigned, there's no need to go through the
// effort to validate or assign its value.
if ( align === undefined ) {
if ( props.attributes.align === undefined ) {
return <BlockListBlock { ...props } />;
}

let wrapperProps = props.wrapperProps;
if (
validAlignments.some( ( alignment ) => alignment.name === align )
) {
wrapperProps = { ...wrapperProps, 'data-align': align };
}

return <BlockListBlock { ...props } wrapperProps={ wrapperProps } />;
return (
<BlockListBlockWithDataAlign
block={ BlockListBlock }
props={ props }
/>
);
},
'withDataAlign'
);
Expand Down