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

Jump to bottom

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

Merged
merged 6 commits into from
Aug 1, 2023
Merged

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

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
165e6c7
Initial commit:
ramonjd Jul 26, 2023
f6087c7
Try grabbing controlled blocks from parent post content block
andrewserong Jul 27, 2023
6fbe676
Cache `selectedClientId`
ramonjd Jul 31, 2023
2764baa
Rename hasParentCoreBlocks to isBlockWithinPattern
ramonjd Jul 31, 2023
b22b26e
Removing while loop since we're already fetching the post content par…
ramonjd Aug 1, 2023
f2f7194
Reinstating while loop because it can deal with nested blocks
ramonjd Aug 1, 2023
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
View file
Open in desktop
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