Footnotes: disable based on post type #52934
Conversation
ellatrix
added
[Block] Footnotes
|
Size Change: +280 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Besides a few minor notes, the fix tests well for me. Thank you, @ellatrix!
I like that block context awareness allows the feature to work in Site Editor when editing pages.
I'm leaning towards keeping the format context experimental/private for the moment and finalizing it for WP 6.4.
| if ( postType !== 'post' && postType !== 'page' ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Note for follow-up: We should check canInsertBlockType here as well. Disabling the block from Preferences results in broken behavior.
There was a problem hiding this comment.
Not sure if we can do that. What If some block disables footnotes to be inserted as inner blocks? That doesn't mean footnote anchors can't be inserted.
There was a problem hiding this comment.
So maybe what we should check here is well is just if footnotes is registered at all? But I guess the preference doesn't unregister it.
There was a problem hiding this comment.
We'll need to check if the footnotes block is disabled; it doesn't have to be canInsertBlockType.
There was a problem hiding this comment.
Looks like that is in the editPostStore, which would create a dependency on the edit-post packages for block-library. 🤔
There was a problem hiding this comment.
Added a registration check in 185c7b3, but that doesn't fix the preferences problem. Not sure if we should depend on the edit post package. Maybe the footnotes block doesn't belong in the block library package. Or is it fine to add the dependency?
|
Just mentioning here as well that there's a filter in |
| @@ -11,8 +11,7 @@ import BlockContext from '../block-context'; | |||
|
|
|||
| const DEFAULT_BLOCK_CONTEXT = {}; | |||
|
|
|||
| // eslint-disable-next-line no-restricted-syntax | |||
| export const usesContextKey = 'usesContext' + Math.random(); | |||
| export const usesContextKey = Symbol( 'usesContext' ); | |||
|
Flaky tests detected in 75330ff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5658846772
|
There was a problem hiding this comment.
Great stuff!
✅ Footnotes and footnote revisions work as expected on page and post types
✅ Tested in templates and custom post types
I found a couple of issues:
- Open up a page that has footnotes and try to add some more. The footnote block will be duplicated. (only in editor, not in frontend)
2023-07-26.10.18.09.mp4
- Create a new pattern. You shouldn't be able to add footnotes when editing the pattern itself because it's a template. However if you add it to the page and try to insert a footnote, it borks
2023-07-26.10.20.31.mp4
I can reproduce this on trunk so it's probably better to fix separately. |
I think it's because Would using
I should have specified "synced" patterns, which are inside E.g., something like this works for me so far for both issuesdiff --git a/packages/block-library/src/footnotes/format.js b/packages/block-library/src/footnotes/format.js
index 2086005a50..4f457d570d 100644
--- a/packages/block-library/src/footnotes/format.js
+++ b/packages/block-library/src/footnotes/format.js
@@ -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
@@ -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, {
+ id: postId,
+ } );
const { selectionChange, insertBlock } =
useDispatch( blockEditorStore );
@@ -58,6 +62,16 @@ export const format = {
return null;
}
+ // Check if the selected block lives within a pattern.
+ if (
+ getBlockParentsByBlockName(
+ getSelectedBlockClientId(),
+ 'core/block'
+ ).length > 0
+ ) {
+ return null;
+ }
+
if ( postType !== 'post' && postType !== 'page' ) {
return null;
}
@@ -86,19 +100,7 @@ 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 );
- }
- }
+ 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
|
I was only thinking that, depending on what @ellatrix thinks would be the best fix, we might need the added context introduced in this PR to address it. |
|
@ramonjd That's an interesting fix! Do you mind to open a PR as a follow-up so it's properly attributed and and check the performance implications? |
|
I agree. Let's address two issues @ramonjd mentioned separately; those two edge cases also exist on the trunk. |
Thanks for checking my logic! Definitely happy to do a follow up to this one. I might need both your 👀 to double check 🙇🏻 |
* Footnotes: disable based on post type * Address feedback * Fix typo * Format: disable if block is not registered * Lock usesContext api * Use Symbol instead of Math.random
|
I just cherry-picked this PR to the update/packages-wp-6-3-RC3 branch to get it included in the next release: adc34b3 |
ramonjd
removed
the
Backport to WP 6.7 Beta/RC
* Patterns: Enable focus mode editing (#52427) * PreventDefault when isComposing is true. apply patch from t-hamano. (#52844) see: #52821 (comment) * List View: Ensure onDrop does not fire if there is no target (#52959) * I18N: Add missing Gettext wrapper on strings in Edit Post overview sidebar (#52971) * I18N: Add missing gettext wrapper * Add context to disambiguate 'Outline' that is commonly used on borders. * Footnotes: disable based on post type (#52934) * Footnotes: disable based on post type * Address feedback * Fix typo * Format: disable if block is not registered * Lock usesContext api * Use Symbol instead of Math.random * Patterns Browse Screen: Fix back button when switching between categories (#52964) * Patterns: Allow orphaned template parts to appear in general category (#52961) * Spacing presets: fix bug with select control adding undefined preset values (#53005) * Site Editor: Fix canvas mode sync with URL (#52996) * Check if spacing tool is defined before displaying controls. (#53008) * Check if spacing tool is defined before displaying controls. * Don't show sides if spacing type false * Improve consistency of the Post editor and Site editor Document actions (#52246) * Remove redundant shortcut button. * Fix focus and hover style and improve consistency. * Rename post document-title and improve CSS consistency. * Site Editor: Fix the typo in the title label map (#53071) * Fix patterns search crash: check for existence of defaultView before attempting to get styles (#52956) * backport paging bug fixes (#53091) --------- Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Hiroshi Urabe <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Pedro Mendonça <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: Andrea Fercia <[email protected]>
|
Yeah I enjoy this PR because I am going to use footnotes block in my "Researches" CPT. Where each Research template is going to have references in for each of my researches that are written in the Post Content. |
|
Sorry, I don't quite understand the need for restricting footnote use here. It's a text format and metadata block, I don't see how allowing this on custom post types that support the editor would cause issues. I'm was going to utilize this on a site that will be publishing lengthy case studies with footnotes. I'm also very confused by the urgency of this; hard coding the post type check into the block and format, rather than implementing a setting/support/filter that allows custom post types to opt into them. |
Both the urgency and excessive caution were due to the fact that we were well into RC (Release Candidate) stage for WordPress 6.3. It's totally fair of you to disagree, but I'm stressing this so that it's clear that everyone in the conversation was aware of the tradeoffs (including thoughts like "well, it's just a text format, what harm could it do to extend its reach?" or "what's wrong with adding an API to enable it at this point?"). It's also important to note that the option to altogether punt footnotes until 6.4 was absolutely on the table for a long time, and we only kept the feature after we reached enough stability — but keeping it restricted to posts and pages was part of the conditions for that to happen. I can say I'm much more comfortable with "soft launching" footnotes in this way rather than doing something that felt too rushed or rather than not launching at all. :) |
What if users want that feature wherever Gutenberg blocs are available? What is the workaround for this if we, endusers want it, please? |
|
For other readers, this question has been addressed in the issue for supporting CPTs: |
What?
For now, let's restrict the footnotes block and format to posts and pages.
Why?
We don't want people to be adding footnote anchors outside of the post content. For now we also don't want custom post types to add footnotes, but we can enable that in a future release.
How?
We need a
usesContextsetting for format types as well. Not sure if we should immediately ship this as a stable API though. What do you think?In the future it would be good to disable block insertion as well, but this is not possible with the current block.json APIs. Maybe we should expand
parent?Testing Instructions
In the site editor, try adding an anchor in a template. The button should not be there. The block is insertable, but it will have a message that is doesn't work.
Testing Instructions for Keyboard
Screenshots or screencast