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

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 1 commit
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
35 changes: 19 additions & 16 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 @@ -16,6 +16,7 @@ import {
} from '@wordpress/block-editor';
import { useSelect, useDispatch, useRegistry } from '@wordpress/data';
import { createBlock, store as blocksStore } from '@wordpress/blocks';
import { useEntityBlockEditor } from '@wordpress/core-data';

/**
* Internal dependencies
Expand All @@ -34,23 +35,26 @@ export const format = {
'data-fn': 'data-fn',
},
contentEditable: false,
[ usesContextKey ]: [ 'postType' ],
[ usesContextKey ]: [ 'postType', 'postId' ],
edit: function Edit( {
value,
onChange,
isObjectActive,
context: { postType },
context: { postType, postId },
} ) {
const registry = useRegistry();
const {
getSelectedBlockClientId,
getBlockRootClientId,
getBlockName,
getBlocks,
getBlockParentsByBlockName,
} = useSelect( blockEditorStore );
const footnotesBlockType = useSelect( ( select ) =>
select( blocksStore ).getBlockType( name )
);
const [ blocks ] = useEntityBlockEditor( 'postType', postType, {
Copy link
Member

Choose a reason for hiding this comment

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

blocks is only used in the onClick callback. Is there a selector that we could run there instead?

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 I was just writing the comment below!

Looking into it now. Thanks!

Copy link
Member Author

@ramonjd ramonjd Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I need to look further, but I'm having trouble finding an alternative that gives me the rendered block list for a page in the site editor. It's definitely a limitation of my knowledge, I don't spend much time in core-data 😄

The blocks property is available outside the click handler, but not within it for example

// returns the right block list outside onClick but `undefined` within it
getEditedEntityRecord(
					'postType',
					postType,
					postId
				)?.blocks

I can see that we get the edited entity record with raw content, whose blocks could be parsed. Not sure if that's very performant either, of if we'd get the necessary clientId of the footnotes block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that we get the edited entity record with raw content, whose blocks could be parsed. Not sure if that's very performant either

I think using getEditedEntityRecord gets us closer to the desired end state. From looking at useEntityBlockEditor, it currently does the parsing here if blocks isn't available, so performance-wise, if we copy/pasted (or followed) a similar approach in the onClick callback, it'd be no slower than using useEntityBlockEditor, I'd think.

if we'd get the necessary clientId of the footnotes block.

It doesn't look like we need the clientId of the footnotes block, rather, we're just checking whether it exists. So I think we should be fine using the parsed content 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

if we'd get the necessary clientId of the footnotes block.

Oh, wait, that does seem to be problem unfortunately! Sorry I missed that in my first pass, without that clientId, we don't have selection move down to the footnote block 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this in the first version only to kick things off.

@ellatrix identified potential performance implications:

useEntityBlockEditor would mean it's re-rendering for any other block changes, it would be best to call a selector in the event handler.

Also I noticed that the footnotes linger when I create a synced pattern from blocks that already have footnotes.

I believe this is also an existing issue, but just making a note here that maybe we could warn folks that their footnotes will no longer work if they create a pattern, and then strip the footnotes? Not sure what the best UX would be.

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

Expand All @@ -62,6 +66,16 @@ 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 (
getBlockParentsByBlockName(
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
getSelectedBlockClientId(),
'core/block'
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Is there any way to check for this without hardcoding the block name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question.

I'm not sure about that, but maybe @glendaviesnz or @aaronrobertshaw might have a better idea about to detect whether a block is inside a pattern?

Context for those folks: here we want to know to know if any of a block's parents are patterns by filtering parents using core/block.

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 I just discovered isReusableBlock

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 I just discovered isReusableBlock

Doesn't help here 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, isReusableBlock hard codes the core/block name as well.

No alternatives spring to mind but @glendaviesnz has explored this area more so might have better ideas.

Copy link
Contributor

Choose a reason for hiding this comment

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

To-date I have only used isReusableBlock sorry, I don't think there is currently an alternative but maybe it is worth adding a new selector along the lines of isParentSyncedPattern?

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 don't think there is currently an alternative but maybe it is worth adding a new selector along the lines of isParentSyncedPattern?

Sounds like a useful follow up. Thank you!

).length > 0
) {
return null;
}

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

// BFS search to find the first footnote block.
let fnBlock = null;
{
const queue = [ ...getBlocks() ];
while ( queue.length ) {
const block = queue.shift();
if ( block.name === name ) {
fnBlock = block;
break;
}
queue.push( ...block.innerBlocks );
}
}
// Finds the first footnote block.
Copy link
Member Author

@ramonjd ramonjd Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is interesting.

Pursuant to the comment above, getEditedEntityRecord kinda works:

				const blocks = getEditedEntityRecord(
					'postType',
					postType,
					postId
				)?.blocks;

				// Finds the first footnote block.
				let fnBlock = blocks?.find( ( block ) => block.name === name );

But first we have to interact with the page itself, e.g., by editing the content in any way

2023-07-27.09.10.54.mp4

postType and postId are available in the callback on first render, so it isn't that I believe

But we're getting closer 😄

Is there a way to trigger this interactivity programmatically? 🤷🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I'm hugely knowledgeable about the core-data stuff either, but would getEntityRecord work for retrieving the initial state?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think part of the problem might be that in order for the selectionChange() call below to work, we might need to somehow get the same list of blocks (and therefore clientIds) that gets set within the parent post content block's useInnerBlocks here (the post content block's controlled inner blocks).

From playing around locally, when I've attempted to call getEditedEntityRecord or getEntityRecord to grab the data I need, we wind up with a duplicate of the blocks, rather than a reference to the existing Footnotes block in the editor 🤔

let fnBlock = blocks?.find( ( block ) => block.name === name );

// Maybe this should all also be moved to the entity provider.
// When there is no footnotes block in the post, create one and
Expand Down