-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Only show transforms to blocks that can be inserted on the root block; Order transforms by frecency; #7184
Only show transforms to blocks that can be inserted on the root block; Order transforms by frecency; #7184
Conversation
33a2421
to
2ae5195
Compare
2ae5195
to
794d6ca
Compare
This PR was rebased, to contain the last changes to the transforms mechanism. It is ready for a look. |
794d6ca
to
d750345
Compare
Any reason why this has not been merged yet? |
@ZebulanStanphill right now this is waiting for a code review. Let's get it a milestone and see if we can get some 👀 on this. Thanks for championing it. |
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 looking good! Love that we're re-using the logic that's already in the selectors. Thanks @jorgefilipecosta.
getPossibleBlockTransformations( blocks ), | ||
( block ) => !! itemsByName[ block.name ] | ||
), | ||
( block ) => -itemsByName[ block.name ].frecency, |
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.
We can make it a little more explicit that this is a reverse sort by using orderBy
.
const possibleBlockTransformations = orderBy(
...,
( block ) => itemsByName[ block.name ].frecency,
'desc'
);
const sourceBlockName = blocks[ 0 ].name; | ||
const blockType = getBlockType( sourceBlockName ); | ||
const hasStyles = blocks.length === 1 && get( blockType, [ 'styles' ], [] ).length !== 0; | ||
|
||
if ( ! hasStyles && ( ! allowedBlocks.length || isLocked ) ) { | ||
if ( ! hasStyles && ( ! possibleBlockTransformations.length ) ) { |
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 inner parens ()
are no longer necessary here.
if ( ! hasStyles && ! possibleBlockTransformations.length ) {
( clientId ) => !! getTemplateLock( getBlockRootClientId( clientId ) ) | ||
), | ||
blocks: getBlocksByClientId( clientIds ), | ||
inserterItems: getInserterItems( rootClientId ), |
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.
Have you looked into using canInsertBlockType
instead of getInserterItems
? The latter takes reusable blocks into account which is unnecessary in this case because you can't transform a block into an existing reusable block. It's better that we do as little work as possible to keep the UI feeling responsive.
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 have thought about this and it seemed getInserterItems was the best option.
We don't want just to know if a block can be inserted we want to know all the blocks that can be inserted so we can render the UI. We can not do a simple filter using canInsertBlockType inline because that would create a new array reference each time triggering unnecessary rerenders. So the alternative would be to create a new selector that just returns all the allowed blocks without the shared blocks. Besides the available blocks, we also need the frecency ( to sort the items ) and getInserterItems already provides it.
getInserterItems is cached and used in the inserter which is rerendered each time a block changes so I think each time we use the selector here we are just retrieving the value from the cache. If we had a specific selector we would not be able to share the cache.
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.
Makes sense 👍 – thanks for explaining!
d750345
to
64a1bfe
Compare
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.
Hell yeah 🤙
packages/editor/src/hooks/align.js
Outdated
@@ -23,6 +23,9 @@ import { BlockControls, BlockAlignmentToolbar } from '../components'; | |||
* @return {Object} Filtered block settings | |||
*/ | |||
export function addAttribute( settings ) { | |||
if ( has( settings, [ 'attributes', 'align' ] ) ) { |
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.
Can we add a test case for this, and ideally an inline code comment explaining its purpose?
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 expect settings
to always be non-undefined?
- If yes, we don't need to incur Lodash's extra nullish check on
settings.attributes
, and instead express this ashas( settings.attributes, [ 'align' ] )
- If no, then we have issues below where we assign a new
settings.attributes
on what we're arguing to be a potentiallyundefined
settings object
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.
…ted on the root block
8835c93
to
3ec9d09
Compare
Fixes: #6363
Part of a general polishing to get #6993.
Relates to: #7227
This PR makes sure we can only transform to blocks that can be inserted in the root block.
The transformations are also sorted by frecency (a heuristic used by the inserter) so items transformations should appear in the same order as they appear on the inserter.
Test block: https://gist.github.com/jorgefilipecosta/b958239761a24664685d5efc7ab48fa6
(can be pasted in the browser console)
How has this been tested?
Use a block that sets allowed blocks in the nested area (test block). Verify it is only possible to transform blocks to the allowed blocks (list and quote).
Test both single and multi-block transforms and verify the restriction applies to transforms rendered on the block toolbar and to transforms presented on the side menu.
Verify that if the allowed blocks restriction is applied via PHP filters, the transforms also respect it.
e.g:
Verify the order blocks appear on the transformations list is the same as the order they appear on the inserted.