-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Footnotes: disable for synced patterns and prevent duplication for pages in site editor #53003
Conversation
- Prevent footnote creation withing core/block - Only insert a footnote if one isn't found in the entity block list
} = useSelect( blockEditorStore ); | ||
const footnotesBlockType = useSelect( ( select ) => | ||
select( blocksStore ).getBlockType( name ) | ||
); | ||
const [ blocks ] = useEntityBlockEditor( 'postType', postType, { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤞
There was a problem hiding this comment.
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 🤔
} = useSelect( blockEditorStore ); | ||
const footnotesBlockType = useSelect( ( select ) => | ||
select( blocksStore ).getBlockType( name ) | ||
); | ||
const [ blocks ] = useEntityBlockEditor( 'postType', postType, { |
There was a problem hiding this comment.
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.
Size Change: +346 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
queue.push( ...block.innerBlocks ); | ||
} | ||
} | ||
// Finds the first footnote block. |
There was a problem hiding this comment.
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? 🤷🏻
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🤔
Update: Hope you don't mind @ramonjd, but I've pushed a change: I've had a go at attempting to grab the controlled inner blocks of the parent post content block in f6087c7 — I think this means we can avoid having to call Do feel free to revert if this approach is no good, of course! |
Flaky tests detected in f2f7194. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5721605092
|
Not at all. Thanks for stepping in here. I'll try to come back to it tomorrow (secretly hoping it'll be "magically" fixed overnight ) |
That was happening in trunk as well, so I think we're good. Thanks for looking at this while I was out. Using Let's see what @ellatrix and others think. |
if ( | ||
getBlockParentsByBlockName( | ||
getSelectedBlockClientId(), | ||
'core/block' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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!
Get hasParentCoreBlocks using separate useSelect call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still doesn't appear to be redirecting focus correctly to the found footnotes block within the page editor within the site editor, so it might not be quite right yet.
That was happening in trunk as well, so I think we're good.
Agreed 👍
This is working nicely for me and the code looks neat. You might want to get a final review from @ellatrix before landing, but this covers things nicely for now IMO:
✅ Footnotes cannot be added in synced patterns edited within the post editor
✅ Footnotes cannot be added in synced patterns within the site editor
✅ For pages edited within the site editor, duplicate Footnotes blocks are no longer appearing
✅ Otherwise editing footnotes appears to be unaffected
Just left a couple of tiny non-blocking nits / questions, but nothing urgent.
LGTM! ✨
|
||
while ( | ||
rootClientId && | ||
getBlockName( rootClientId ) !== 'core/post-content' | ||
getBlockName( rootClientId ) !== POST_CONTENT_BLOCK_NAME |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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 👍🏻
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great!
…ent in the `getBlockParentsByBlockName` call above
…ges in site editor (#53003) * Initial commit: - Prevent footnote creation withing core/block - Only insert a footnote if one isn't found in the entity block list * Try grabbing controlled blocks from parent post content block * Cache `selectedClientId` Get hasParentCoreBlocks using separate useSelect call. * Rename hasParentCoreBlocks to isBlockWithinPattern Add comments * Removing while loop since we're already fetching the post content parent in the `getBlockParentsByBlockName` call above * Reinstating while loop because it can deal with nested blocks --------- Co-authored-by: Andrew Serong <[email protected]>
I just cherry-picked this PR to the update/second-round-6-3-rc3 branch to get it included in the next release: ef4f1f2 |
* Top toolbar: Fix issues with save button overlap, and plugin buttons (#53101) * Shorten the width of the top toolbar overlay and make doc title smaller. * Add a scrim and a margin to handle plugin buttons better. * Remove block tools back compat component schedule for deprecated in 6.3 (#53115) * Removes usage of BlockToolsBackCompat * Remove unwanted BlockTools from Nav sidebar * Footnotes/RichText: fix getRichTextValues for deeply nested blocks (#53034) * Defer to preceding handlers in command palette keyboard shortcut (#53001) * Image block: fix image size at wide and full width (#53184) * Fix regression with Edit site Navigate regions (#52940) * Make the navigabel region wrap the inert sidebar. * Adjust animation. * Fix not expanding pattern in page editor (#53169) --------- Co-authored-by: Aaron Robertshaw <[email protected]> * Footnotes: fix published preview (#53072) * Footnotes: fix published preview * remove var dump * Fix php lint * PHP lint * Address feedback * Add e2e test * Footnotes: disable for synced patterns and prevent duplication for pages in site editor (#53003) * Initial commit: - Prevent footnote creation withing core/block - Only insert a footnote if one isn't found in the entity block list * Try grabbing controlled blocks from parent post content block * Cache `selectedClientId` Get hasParentCoreBlocks using separate useSelect call. * Rename hasParentCoreBlocks to isBlockWithinPattern Add comments * Removing while loop since we're already fetching the post content parent in the `getBlockParentsByBlockName` call above * Reinstating while loop because it can deal with nested blocks --------- Co-authored-by: Andrew Serong <[email protected]> * Footnotes: add missing _ in revision field filter (#53135) * Footnotes: add missing _ in revision field filter * Use correct hook name * Revert prefixing callback names * don't display BlockContextualToolbar at all in contentonly locking (#53110) * Render the footer conditionally in the global styles sidebar component so that any side effects from the footer wrapper are not rendered, e.g., styles and what not (#53204) Ensure that the precise bottom margin persists if the footer isn't rendered * Pattern: Add getBlockRootClientId call (#53206) --------- Co-authored-by: Joen A <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Mitchell Austin <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]>
What?
A follow up to:
This PR attempts to resolves some issues found while testing:
core/block
blocksWhy?
See #52934 (review)
How?
Testing Instructions
Screenshots or screencast