Inserter: Prevent non-deterministic order of inserter items #34078
Conversation
|
Size Change: +111 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
The test seems reasonable to me, @fluiddot . |
fluiddot
added
[Type] Bug
There was a problem hiding this comment.
Well done! This is a creative yet straightforward solution to this odd problem. Thank you. This looks good to me.
I verified each of the test cases outlined in the PR description. I left one comment regarding naming, but it is not a blocker from my perspective.
I would imagine we should wait for a web team member's input before merging, so I will yield approval to them.
|
Thanks for this fix @fluiddot and @ttahmouch ! 🙇 I wonder if there's room for us to improve the default block order (in a different PR). This jumped out at me because the "more" block didn't seem to make sense in the third position (ahead of, for example, image, video, and list). Any thoughts on this @iamthomasbishop ? I know that we're planning to revamp the inserter soon, but if the order can be improved it would be a pretty easy fix. Related previous discussion: wordpress-mobile/gutenberg-mobile#1105 |
I think I agree with the sentiment behind this. I suppose I assumed the order would be self-correcting based on the frecency. However, it never quite made sense to me when attempting the dis/allow list implementation why the default order of the block types differed between |
The order is different for the list of most common blocks between web and mobile, so it seems correct how those blocks are presented in the inserter. However, I agree with the sentiment that they should be presented in the same order on both platforms. |
gziolo
added
the
[Feature] Inserter
| ]; | ||
| expect( items.map( ( { name } ) => name ) ).toEqual( expectedResult ); | ||
| expect( items.map( ( { id } ) => id ) ).toEqual( expectedResult ); |
gziolo
changed the title
The default block order in the native version is driven by the block registration order defined here: gutenberg/packages/block-library/src/index.native.js Lines 236 to 270 in 20d9c54 And on the web version is here: gutenberg/packages/block-library/src/index.js Lines 119 to 189 in 20d9c54 I'm wondering if we could unify them 🤔 , in the meantime and as a quick fix, we could update the list on the native version to match the same order as in the web. |
|
This PR has approvals for both versions native (approval #1 and approval #2) and web (approval #3), so it's ready to be merged. |
|
@fluiddot, it looks like the code for registering core blocks could be unified between the native mobile and web now that most of the blocks work on both platforms. A good example would be to extract common blocks (the group in the web version) to a new file and import them in both versions of |
That's a great idea, in the native version we would still require to filter out the unsupported blocks but I agree that extracting the common blocks will make the order consistent between web and native 🎊 . |
|
You can also keep some of the imported blocks behind the |
Yep, that would help for sure. Let's try this option and see how it looks like. |
* Prevent non-deterministic order of inserter items * Add block variations to getInserterItems unit test * Display core block variations before non-core blocks * Rename toTyped to groupByType
* Release script: Update react-native-editor version to 1.59.0 * Release script: Update with changes from 'npm run core preios' * Update release notes * Release script: Update react-native-editor version to 1.59.1 * Release script: Update with changes from 'npm run core preios' * Mobile - Global styles - Add color to the list of styles to include in the filter (#34000) * Rich text - toTree - Add optional chaining in replacements before accessing its type (#34020) * Update 1.59.1 changelog * Reinstate Unreleased section of changelog * Release script: Update react-native-editor version to 1.59.2 * Release script: Update with changes from 'npm run core preios' * Inserter: Prevent non-deterministic order of inserter items (#34078) * Prevent non-deterministic order of inserter items * Add block variations to getInserterItems unit test * Display core block variations before non-core blocks * Rename toTyped to groupByType * Update react-native-editor CHANGELOG * [RNMobile] Fix missing block title of core/latest-posts block (#34116) * Update react-native-editor CHANGELOG * [TEST] Use npm install in RN E2E Tests (iOS) workflow * Revert "[TEST] Use npm install in RN E2E Tests (iOS) workflow" This reverts commit 2b7e0e6. Co-authored-by: jhnstn <jason@readysetandco.com> Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com> Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com>
|
Looking at the performance graph for the "inserter hover" metric, it seems it started to be a bit less performant around this commit https://codehealth.vercel.app While it's not certain 100%, it sounds like this commit is the most likely in that range of commits to impact it. The increase is not big enough to be very concerning but might be a good indicator to at least try to debug/check. |
That's interesting, after checking the graph (see attached screenshot), I see that the commit that introduced this change
|
This PR takes the changes created by @ttahmouch from PR #34062, thanks for the help on this ❤️!
Description
We've noticed that using the function
Array.prototype.sortcan produce different results on the native version, actually on Android due to the use of Hermes, it's generating a different list than iOS (which usesjavascriptcore). Per the documentation of the function, it looks like the function itself is not stable until ES2019 so it would be nice to figure out a replacement to prevent non-deterministic results.Specifically, we've identified this issue in the
getInserterItemsselector (reference), so a solution has been applied for this purpose by replacing the use ofsortwith the calculation of the core and non-core block lists usingreduce. This directly affects the logic related to prioritizing core blocks' display in the inserter menu.How has this been tested?
Unit test
Block variations case has been added to the unit test "getInserterItems with core blocks prioritization" to assure the proper order of blocks in the inserter menu.
Web version
Verify that inserter items order match the production version
For this purpose, we can compare the current order of a WP.com site with a local instance of Gutenberg using the changes from this PR.
NOTE: In a WP.com site the inserter menu shows more items than a local instance, however, the extra items should always be displayed at the end of the list.
Additionally, we should verify that the inserter menu displayed within the blocks matches the same order as the blocks are registered.
gutenberg/packages/block-library/src/index.js
Lines 119 to 188 in 34fbec6
Native version
Similar to the web version, we have to verify that the inserter items order remains the same as in the production version, in this case, we would need to use the iOS version because the Android one is not displaying the correct order. Additionally, we need to verify that both iOS and Android versions display the same order.
Verify that inserter items order match the production version
Verify IOS and Android match the inserter items order
gutenberg/packages/block-library/src/index.native.js
Lines 237 to 269 in 947816e
NOTE: The following blocks are not displayed in the inserter menu:
missingcolumnclassicbuttonsocialLinkreusableBlockThe block
core/searchis displayed at the bottom because it's being included as a core variant.gutenberg/packages/block-library/src/search/variations.js
Lines 6 to 12 in 947816e
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).