Prioritize core blocks in the inserter #28945
Merged
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
|
Size Change: +15 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
ntsekouras
changed the title
ntsekouras
added
[Feature] Inserter
ntsekouras
commented
Feb 11, 2021
| @@ -43,22 +43,14 @@ export function BlockTypesTab( { | |||
| }, [ items ] ); | |||
|
|
|||
| const itemsPerCategory = useMemo( () => { | |||
| const getCategoryIndex = ( item ) => { | |||
There was a problem hiding this comment.
This seems to do nothing useful. I don't think I'm missing something here, but I'l like a sanity check :)
There was a problem hiding this comment.
The original intent for this was to sort the categories, though they might be sorted elsewhere now :)
There was a problem hiding this comment.
Thanks for picking this up! I wasn't sure if this behavior was intentional or not.
I could definitely see/agree with the argument that core blocks should be shown first. I do wonder if we want to give plugins the option to set the priority for whether their blocks appear before or after core blocks, but I think thats a separate discussion and follow up if necessary.
The changes here look good to me, although I'm not personally certain about the current usage of the getCategoryIndex.
Anyways, thanks again!
ntsekouras
force-pushed
the
add/insterter-core-blocks-prioritization
branch
from
14f778f
to
9f2ceb9
Compare
I agree that would be a separate discussion and follow up if necessary. Thanks for testing @Addison-Stavlo ! |
|
@Addison-Stavlo did you also test-run this? Just wondering if the PR can be merged. :-) Big thanks @ntsekouras for jumping in to fix this and also for adding tests! |
Addison-Stavlo
approved these changes
Feb 16, 2021
There was a problem hiding this comment.
@Addison-Stavlo did you also test-run this? Just wondering if the PR can be merged. :-)
Yup! I was just tentative to approve at first because I wasn't very sure about the category sorting removal, nor was I certain whether or not 3rd party blocks were intended to come first or not.
After more thought, some more testing, and seeing no objections - everything looks good and tests well from my end! 🚢
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
3rd party blocks appear before core blocks in the sidebar inserter. This happens because third party blocks can be registered earlier than the core blocks (usually by using the
initaction), thus affecting the display order.This PR ensures core blocks are prioritized in the returned results.
Before
After
Related: Automattic/wp-calypso#46981
Checklist: