-
Notifications
You must be signed in to change notification settings - Fork 618
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
fix: refactor suggested-blocks toolbox to use JSON definition #1632
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
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.
Hello, thank you for the PR! It needs a few changes before we can merge it, so I'll include some resources and links below for you to explore.
- It would be great if you could test this code before submitting. One way you can do this is to copy the code to the
test/index.js
file and make sure it works there for this plugin. More info about verifying changes - I'm going to update your PR description to include a link to the issue it would fix, and update your PR title to follow conventional commits. No action needed on your end for this, just FYI.
- Make sure you sign the CLA, as without it we won't be able to merge your code. Instructions are provided by the googlebot.
- I'm going to close Update README.md #1626 since it looks like this is an updated version.
Thank you! Please let us know if you have any questions or would like some additional help getting this merged.
Maribeth
|
||
// Insert the new categories into the existing toolboxCategories array | ||
return [ | ||
...toolboxCategories.slice(0, -1), |
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 toolboxCategories
object is here. It's a json object, not an array. You need to modify the toolboxCategories.contents
array. Since we're just adding to the end of the array, you can use push
or destructuring but you don't need to use slice
.
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.
Sorry, after looking at this again, we actually don't need to use the toolboxCategories
from @blockly/dev-tools
at all. This code is in the README where we are showing developers how to use this plugin. The toolboxCategories
object is provided as a convenience when developing plugins, but it's not intended for use outside of plugins, so we shouldn't include it in the README.
So what we should do is show a snippet of a self-contained toolbox. Since it's just an example, the toolbox can be pretty bare-bones apart from our two special categories we're showing from this plugin. Here's an example, which you can use or come up with your own.
const toolbox = {
'kind': 'categories',
'contents': [
// Your own categories and blocks here...
{
'kind': 'category',
'name': 'Frequently Used',
'custom': 'MOST_USED',
},
{
'kind': 'category',
'name': 'Recently Used',
'custom': 'RECENTLY_USED',
},
],
};
This toolbox only has two categories, which isn't super useful, but a developer can learn how to specify toolboxes in general from other documentation.
// Define the toolbox as a JSON object | ||
const toolbox = { | ||
kind: 'categoryToolbox', | ||
categories: getNewCategories() |
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.
Don't feel obligated to keep the getNewCategories
method. It's probably cleaner to declare a new variable called toolbox
without needing to call this method.
Hello @martinyis ! Are you still interested in working on this? |
Hello! I'm going to go ahead and close this since there hasn't been activity =) But if you're still interested in working on this feel free to reopen! |
This PR refactors the toolbox definition in the existing code to use a JSON definition, rather than manipulating the existing XML string. The getNewCategories function now returns an array of JSON objects representing the two new categories. The toolbox object defines a categoryToolbox with an array of categories obtained by calling the getNewCategories function. The new toolbox definition is then passed to the Blockly.inject method to initialize the workspace. This change improves readability and maintainability of the code, and also makes it easier to modify the toolbox definition in the future.
fixes #1625