-
Notifications
You must be signed in to change notification settings - Fork 3
Add basic block inserter features: grid, search, icons, ordering #196
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
Conversation
1115994 to
7b5cabb
Compare
| import Foundation | ||
|
|
||
| /// A protocol for items that can be searched | ||
| protocol Searchable { |
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.
Disclaimer: other code in PR is written by hand, but this file is entirely generated. It uses Levenshtein distance for fuzzy matching, and does scoring depending on weight of searchable fields. It seems to work pretty well, and we do need it in the inserter.
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.
Searching worked relatively well from my testing. I noted one thing while searching: we are not honoring the inserter supports attribute for blocks. Blocks setting this to false should not appear in the block inserter or its search results.
| * | ||
| * @return {string|null} The serialized icon string or null. | ||
| */ | ||
| function getBlockIcon( blockType ) { |
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.
OK, I generated this too. I might need some help verifying if it makes sense (it works).
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.
Yes, it makes sense. It seems necessary given the desired outcome of providing SVG strings to the native host app. I cannot think of a better alternative or specific problems with the approach at this time.
src/utils/bridge.js
Outdated
| keywords: blockType.keywords || [], | ||
| }; | ||
| } ); | ||
| const blocks = getSerializedBlocks(); |
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 haven't reviewed all the code yet, but wanted to note we need to rebase to include #198 and adapt this implementation so that the remote editor works.
Good news is that the native inserter does work with the remote editor in my quick merge conflict resolution on my local machine.
a54f386 to
b5f3d7b
Compare
b5f3d7b to
a55adca
Compare
The static import resulted in `window.wp` references prior them being defined. This problem is unique to the remote editor, which asynchronously loads site-specific `@wordpress` module scripts. Using a dynamic import within the function ensures the reference does occur until after the globals are defined.
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.
Approving as this works well from my testing and other PRs build atop this. My inline comments note one issue we should address regarding inserter block inclusion.
| import Foundation | ||
|
|
||
| /// A protocol for items that can be searched | ||
| protocol Searchable { |
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.
Searching worked relatively well from my testing. I noted one thing while searching: we are not honoring the inserter supports attribute for blocks. Blocks setting this to false should not appear in the block inserter or its search results.
| * | ||
| * @return {string|null} The serialized icon string or null. | ||
| */ | ||
| function getBlockIcon( blockType ) { |
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.
Yes, it makes sense. It seems necessary given the desired outcome of providing SVG strings to the native host app. I cannot think of a better alternative or specific problems with the approach at this time.
| return { | ||
| name: blockType.name, | ||
| title: blockType.title, | ||
| description: blockType.description, | ||
| category: blockType.category, | ||
| keywords: blockType.keywords || [], | ||
| icon: getBlockIcon( blockType ), | ||
| }; |
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.
As noted in my comment regarding search, we likely need to include the inserter supports attribute so that we can honor that setting.
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 added it as a bug and will address in the upcoming PRs. Thanks!
|
Noting that this may be a breaking change for |
Changes
BlockInserterViewand a set or related types to render the blocks in a grid (closes CMM-857)Testing Instructions
Note: you can not yet tap to select a block.
Technical Notes
I added SVGKit to render block icons. It seems like the only viable option.
Limitations
BlockInserterViewModelestablishes some mobile-specific ordering for blocks, but I opened a separate ticket (CMM-875) to further enhance it (see screenshots/comments). I'm considering implementing it on the JS level.Screenshots