-
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
Add new API to allow inserter items to be prioritised #50510
Changes from 33 commits
d3d0ed6
1261536
e8d2c78
d72bb33
66b4a02
d987b29
4c1797e
d47e6c8
07aec20
25b18f2
264177a
509c362
1d81ef4
1523f1c
eeb586d
be2e9d6
5d6f1b5
1162a72
51e5d3c
db4568f
96b70f3
21c12e1
9fde1a0
ef93a6d
b163447
6bf6d37
8cb75c8
d0ddf60
836b407
a954bae
a787e9d
45bd5ac
01ad80a
784bffb
259a23d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,9 +73,13 @@ function UncontrolledInnerBlocks( props ) { | |
const { | ||
clientId, | ||
allowedBlocks, | ||
prioritizedInserterBlocks, | ||
__experimentalDefaultBlock, | ||
__experimentalDirectInsert, | ||
template, | ||
templateLock, | ||
templateInsertUpdatesSelection, | ||
__experimentalCaptureToolbars: captureToolbars, | ||
Comment on lines
+77
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need these ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this question. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The question is "why are we adding all these extra props when this PR is about |
||
orientation, | ||
renderAppender, | ||
renderFooterAppender, | ||
|
@@ -97,7 +101,17 @@ function UncontrolledInnerBlocks( props ) { | |
|
||
const context = useBlockContext( clientId ); | ||
|
||
useNestedSettingsUpdate( clientId, allowedBlocks, templateLock ); | ||
useNestedSettingsUpdate( | ||
clientId, | ||
allowedBlocks, | ||
prioritizedInserterBlocks, | ||
__experimentalDefaultBlock, | ||
__experimentalDirectInsert, | ||
templateLock, | ||
captureToolbars, | ||
orientation, | ||
layout | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this PR adding all these other settings and not only the core feature There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because not doing that broke the Mobile implementation (cc @fluiddot). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah cool! |
||
); | ||
|
||
useInnerBlockTemplateSync( | ||
clientId, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ const pendingSettingsUpdates = new WeakMap(); | |
* @param {string} clientId The client ID of the block to update. | ||
* @param {string[]} allowedBlocks An array of block names which are permitted | ||
* in inner blocks. | ||
* @param {string[]} prioritizedInserterBlocks Block names and/or block variations to be prioritized in the inserter. | ||
ntsekouras marked this conversation as resolved.
Show resolved
Hide resolved
scruffian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {?WPDirectInsertBlock} __experimentalDefaultBlock The default block to insert: [ blockName, { blockAttributes } ]. | ||
* @param {?Function|boolean} __experimentalDirectInsert If a default block should be inserted directly by the | ||
* appender. | ||
|
@@ -40,6 +41,7 @@ const pendingSettingsUpdates = new WeakMap(); | |
export default function useNestedSettingsUpdate( | ||
clientId, | ||
allowedBlocks, | ||
prioritizedInserterBlocks, | ||
__experimentalDefaultBlock, | ||
__experimentalDirectInsert, | ||
templateLock, | ||
|
@@ -64,13 +66,27 @@ export default function useNestedSettingsUpdate( | |
[ clientId ] | ||
); | ||
|
||
// Memoize as inner blocks implementors often pass a new array on every | ||
// render. | ||
const _allowedBlocks = useMemo( () => allowedBlocks, allowedBlocks ); | ||
// Memoize allowedBlocks and prioritisedInnerBlocks based on the contents | ||
// of the arrays. Implementors often pass a new array on every render, | ||
// and the contents of the arrays are just strings, so the entire array | ||
// can be passed as dependencies. | ||
|
||
const _allowedBlocks = useMemo( | ||
() => allowedBlocks, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
allowedBlocks | ||
); | ||
|
||
const _prioritizedInserterBlocks = useMemo( | ||
() => prioritizedInserterBlocks, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
prioritizedInserterBlocks | ||
); | ||
Comment on lines
+69
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: We'll have to remove this hack in the futere if we want to support dynamic |
||
|
||
useLayoutEffect( () => { | ||
const newSettings = { | ||
allowedBlocks: _allowedBlocks, | ||
prioritizedInserterBlocks: _prioritizedInserterBlocks, | ||
templateLock: | ||
templateLock === undefined || parentLock === 'contentOnly' | ||
? parentLock | ||
|
@@ -130,6 +146,7 @@ export default function useNestedSettingsUpdate( | |
clientId, | ||
blockListSettings, | ||
_allowedBlocks, | ||
_prioritizedInserterBlocks, | ||
__experimentalDefaultBlock, | ||
__experimentalDirectInsert, | ||
templateLock, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,7 +150,6 @@ class PrivateInserter extends Component { | |
prioritizePatterns, | ||
onSelectOrClose, | ||
selectBlockOnInsert, | ||
orderInitialBlockItems, | ||
} = this.props; | ||
|
||
if ( isQuick ) { | ||
|
@@ -174,7 +173,6 @@ class PrivateInserter extends Component { | |
isAppender={ isAppender } | ||
prioritizePatterns={ prioritizePatterns } | ||
selectBlockOnInsert={ selectBlockOnInsert } | ||
orderInitialBlockItems={ orderInitialBlockItems } | ||
/> | ||
); | ||
} | ||
|
@@ -426,13 +424,7 @@ export const ComposedPrivateInserter = compose( [ | |
] )( PrivateInserter ); | ||
|
||
const Inserter = forwardRef( ( props, ref ) => { | ||
return ( | ||
<ComposedPrivateInserter | ||
ref={ ref } | ||
{ ...props } | ||
orderInitialBlockItems={ undefined } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose the other option is to keep this API, but instead support passing a custom appender to List View (instead of the showAppender prop). It still doesn't solve the issue where the block itself ends up with different ordering, unless both the block and the list view instance use the same custom appender, so I think what's in this PR is better. It's define once and it works everywhere which means there's less room for errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
is better than the prop, but why would we need to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only if you wanted specific instance X of an inserter to have a specific order. The new implementation in this PR is a blanket application. Any block that uses the new prop on The previous implementation was more targetted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't Edit: no, that's for |
||
/> | ||
); | ||
return <ComposedPrivateInserter ref={ ref } { ...props } />; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably follow up to remove the need for the private wrapper once (and if!) this PR is merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would keep the private wrapper. This shape allows for easy future private APIs without all the boilerplate being re-added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was looking for why we haven't removed the private inserter. IMO, this doesn't seem like a good reason to keep things around just in case we might need them in the future. |
||
} ); | ||
|
||
export default Inserter; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,5 +20,5 @@ export async function getAllBlockInserterItemTitles() { | |
return inserterItem.innerText; | ||
} ); | ||
} ); | ||
return [ ...new Set( inserterItemTitles ) ].sort(); | ||
return [ ...new Set( inserterItemTitles ) ]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this in 8cb75c8. Because it does sorting on the results the tests cannot assert on the priority of the blocks. This helper is "doing it wrong". Instead it should return raw results and the consumer should do the sorting. Let's see which tests break before deciding how to proceed from here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we think this should be it's own sub PR? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
<?php | ||
/** | ||
* Plugin Name: Gutenberg Test InnerBlocks Prioritized Inserter Blocks | ||
* Plugin URI: https://github.com/WordPress/gutenberg | ||
* Author: Gutenberg Team | ||
* | ||
* @package gutenberg-test-inner-blocks-prioritized-inserter-blocks | ||
*/ | ||
|
||
/** | ||
* Registers a custom script for the plugin. | ||
*/ | ||
function enqueue_inner_blocks_prioritized_inserter_blocks_script() { | ||
wp_enqueue_script( | ||
'gutenberg-test-inner-blocks-prioritized-inserter-blocks', | ||
plugins_url( 'inner-blocks-prioritized-inserter-blocks/index.js', __FILE__ ), | ||
array( | ||
'wp-blocks', | ||
'wp-block-editor', | ||
'wp-element', | ||
'wp-i18n', | ||
), | ||
filemtime( plugin_dir_path( __FILE__ ) . 'inner-blocks-prioritized-inserter-blocks/index.js' ), | ||
true | ||
); | ||
} | ||
|
||
add_action( 'init', 'enqueue_inner_blocks_prioritized_inserter_blocks_script' ); |
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.
do we need to document this somewhere else or is this enough?
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.
That's the official docs so I guess that's enough.