feat: basic block factory implementation #1995
Conversation
maribethb
requested review from
rachel-fenichel
and removed request for
a team
| /** | ||
| * Label (non-serializable) field. | ||
| */ | ||
| export const fieldLabel = { |
There was a problem hiding this comment.
A small opinion: block definitions that can be written only in JSON should be. JavaScript definitions should be used for more dynamic blocks.
There was a problem hiding this comment.
This gets complicated if I'm trying to export the block definitions so that they can be registered from a different file than they're defined in. I would have to know whether the thing I'm exporting is a JS definition or a JSON definition, and then call them differently in the register call. I think it's simpler and less prone to error to define and export them all consistently even though that means using JS when I could use JSON.
We have been trying to remove side effects from our files but in core we still register blocks in the file we declare them so I don't have a good pattern to follow. I'm open to suggestions if this is important to you.
|
@BeksOmega ready for review; I believe I addressed the feedback so far and I created a new feature branch for this. Thanks! |
| export const factoryBase = { | ||
| // Base of new block. | ||
| init: function() { | ||
| this.setColour(120); |
There was a problem hiding this comment.
Do we care about using best practices? If so this should probably be using a theme. I won't comment on any other "best practices" things until I know if we care or not.
There was a problem hiding this comment.
Using a theme sounds good, but I'd prefer to do that in a separate PR. I'll also update the toolbox to not just reuse logic_category etc.
I do care about best practices, with the caveat that for some stuff this is just an initial PR to prove the feasibility of using the code generator approach instead of the manual approach used in the old tool, so some of the best practices will be done in later PRs
| if (e.isUiEvent || e.type == Blockly.Events.FINISHED_LOADING || | ||
| mainWorkspace.isDragging()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Generally more predictable + future proof to use allow lists rather than deny lists for change listeners. E.g. google/blockly#6337
There was a problem hiding this comment.
I want pretty much all non-UI events though. I feel like this is a standard pattern we use elsewhere, and a list of every non-UI event would be harder to read and scan for accuracy (easy to forget one).
|
Heya @maribethb do you think you could split this up into a few different PRs? I don't think this is actually comit-wise reviewable because things in the earlier commits change in later commits. |
|
@maribethb can I close this? |
The basics
The details
Resolves
Proposed Changes
Can be reviewed commit-wise.
Basic implementation of developer tools. There is still some placeholder text and things that need to be cleaned up. More work is following this :)
inputblock to be a single block type with a dropdown for the input type. This makes it easier to change input styles.typeblock to be a single block type with a dropdown for the type. This makes it easier to change types.Most of the code currently in
src/index.tswill be rearranged and moved to different files. I have another branch branched from this one where I've started rearranging the logic to use controllers and separate out the dom manipulation code. I wanted to start by validating the CodeGenerator approach instead of starting by creating a bunch of application scaffolding.Since this isn't being published anywhere I think it's fine to go directly into master but can also put it in a branch if that's preferred.
Reason for Changes
Modernizing dev-tools.
Test Coverage
No unit tests yet. We lack infrastructure for running tests in examples/
Documentation
Additional Information
desktop view
mobile view
