Refactor BlockSwitcher - make function component and new specific selector getBlockTransformItems
#27674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
the
[Type] Code Quality
|
Size Change: +614 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Dec 11, 2020
6 tasks
ntsekouras
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
ntsekouras
force-pushed
the
refactor/block-switcher-to-function-component
branch
from
df296eb to
086b72b
Compare
ntsekouras
changed the title
getBlockTransformItems
ellatrix
reviewed
Dec 17, 2020
packages/block-editor/src/components/block-switcher/block-styles-menu.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-switcher/block-styles-menu.js
Outdated
Show resolved
Hide resolved
| </div> | ||
| const BlockSwitcher = ( { clientIds } ) => { | ||
| const { replaceBlocks } = useDispatch( 'core/block-editor' ); | ||
| const { blocks, possibleBlockTransformations, hasBlockStyles } = useSelect( |
There was a problem hiding this comment.
Do we need access to the whole blocks array in the component? If we only need to know the length, then we should let the selector return this more specific information. The more specific the information, the less the selector will trigger the component to re-render.
There was a problem hiding this comment.
Do we need access to the whole blocks array in the component?
Yes
replaceBlocks( clientIds, switchToBlockType( blocks, name ) );
const isSelectionOfSameType = uniq( blocks.map( ( { name } ) => name ) ).length === 1;
| let icon; | ||
| if ( isSelectionOfSameType ) { | ||
| const sourceBlockName = hoveredBlock.name; | ||
| const blockType = getBlockType( sourceBlockName ); |
There was a problem hiding this comment.
The getBlockType call should be in the selector. The more calls you can stuff in the selector, the better, because it's called way less than the render function. :)
Some prior art: c183c32#diff-e0a942d6bad655ae6985e5feb9410ac2c6127356a70142478746c9ef1921bc41R39
There was a problem hiding this comment.
The same goes for the variables above.
There was a problem hiding this comment.
changed here: 0a68ebd
packages/block-editor/src/components/block-switcher/block-transformations-menu.js
Show resolved
Hide resolved
| state.preferences.insertUsage, | ||
| state.settings.allowedBlockTypes, | ||
| state.settings.templateLock, | ||
| getBlockTypes(), |
There was a problem hiding this comment.
Does this change? I'm not sure this function will be called if it does change? The function is only called when the state changes.
| * this item. | ||
| * @property {number} frecency Heuristic that combines frequency and recency. | ||
| */ | ||
| export const getBlockTransformItems = createSelector( |
There was a problem hiding this comment.
Is this new code? I cannot see where it's removed if it's a refactoring.
Not entirely sure if this should be a selector and public API? Cc @youknowriad.
There was a problem hiding this comment.
Yes.
Based on this comment: #27674 (comment)
It's exported so as to be tested. Maybe we should make selective selectors to public API? Currently we expose all of them.
youknowriad
approved these changes
Dec 21, 2020
| isDisabled, | ||
| frecency: calculateFrecency( time, count ), | ||
| }; | ||
| if ( buildScope === 'transform' ) return blockItemBase; |
There was a problem hiding this comment.
It is interesting to note that transforms don't support block variations, maybe it something we should consider.
There was a problem hiding this comment.
Yeah, this is tracked in this issue: #26962
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR makes BlockSwitcher a function component and took the opportunity to break it in smaller pieces as well, as it was quite big.
Based on this comment: #27674 (comment)
I have also created a new selector (
getBlockTransformItems) to get the transform items. PreviouslygetInserterItemswas used and even though it shares some common logic with the new selector, the new one is lighter.There is also a small enhancement to
disabletransformations by taking into account if a block is allowedmultipletimes.Checklist: