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

Footnotes: disable for synced patterns and prevent duplication for pages in site editor #53003

Merged
Merged
Changes from all commits
Commits
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
52 changes: 47 additions & 5 deletions packages/block-library/src/footnotes/format.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ import { unlock } from '../lock-unlock';
const { usesContextKey } = unlock( privateApis );

export const formatName = 'core/footnote';

const POST_CONTENT_BLOCK_NAME = 'core/post-content';
const SYNCED_PATTERN_BLOCK_NAME = 'core/block';

export const format = {
title: __( 'Footnote' ),
tagName: 'sup',
Expand All @@ -44,13 +48,30 @@ export const format = {
const registry = useRegistry();
const {
getSelectedBlockClientId,
getBlocks,
getBlockRootClientId,
getBlockName,
getBlocks,
getBlockParentsByBlockName,
} = useSelect( blockEditorStore );
const footnotesBlockType = useSelect( ( select ) =>
select( blocksStore ).getBlockType( name )
);
/*
* This useSelect exists because we need to use its return value
* outside the event callback.
*/
const isBlockWithinPattern = useSelect( ( select ) => {
const {
getBlockParentsByBlockName: _getBlockParentsByBlockName,
getSelectedBlockClientId: _getSelectedBlockClientId,
} = select( blockEditorStore );
const parentCoreBlocks = _getBlockParentsByBlockName(
_getSelectedBlockClientId(),
SYNCED_PATTERN_BLOCK_NAME
);
return parentCoreBlocks && parentCoreBlocks.length > 0;
}, [] );

const { selectionChange, insertBlock } =
useDispatch( blockEditorStore );

Expand All @@ -62,6 +83,11 @@ export const format = {
return null;
}
ellatrix marked this conversation as resolved.
Show resolved Hide resolved

// Checks if the selected block lives within a pattern.
if ( isBlockWithinPattern ) {
return null;
}

function onClick() {
registry.batch( () => {
let id;
Expand All @@ -86,10 +112,27 @@ export const format = {
onChange( newValue );
}

const selectedClientId = getSelectedBlockClientId();

/*
* Attempts to find a common parent post content block.
* This allows for locating blocks within a page edited in the site editor.
*/
const parentPostContent = getBlockParentsByBlockName(
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
selectedClientId,
POST_CONTENT_BLOCK_NAME
);

// When called with a post content block, getBlocks will return
// the block with controlled inner blocks included.
const blocks = parentPostContent.length
? getBlocks( parentPostContent[ 0 ] )
: getBlocks();

// BFS search to find the first footnote block.
let fnBlock = null;
{
const queue = [ ...getBlocks() ];
const queue = [ ...blocks ];
while ( queue.length ) {
const block = queue.shift();
if ( block.name === name ) {
Expand All @@ -104,12 +147,11 @@ export const format = {
// When there is no footnotes block in the post, create one and
// insert it at the bottom.
if ( ! fnBlock ) {
const clientId = getSelectedBlockClientId();
let rootClientId = getBlockRootClientId( clientId );
let rootClientId = getBlockRootClientId( selectedClientId );

while (
rootClientId &&
getBlockName( rootClientId ) !== 'core/post-content'
getBlockName( rootClientId ) !== POST_CONTENT_BLOCK_NAME
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why do we still need a while loop here if we got the parent post content block earlier?

Copy link
Member

Choose a reason for hiding this comment

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

In other words, if we know if there's a post content block, we can append it there, if there's no such block, append it to '' (the root)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good point.

I'm testing replacing with

					if ( parentPostContent.length ) {
						rootClientId = parentPostContent[ 0 ];
					}

Working okay so far 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to revert this change because adding footnotes to list blocks was failing. I suspect this test doesn't account for nested blocks. Given we're close to 6.3 release, we can do a follow up to see if we can tidy this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The failing test that brought this to our attention was Footnotes › can be inserted in a list for the record. Great to have tests!!

) {
rootClientId = getBlockRootClientId( rootClientId );
}
Expand Down
Loading