General feedback

[sorawee]

There are a bunch of code that I do not understand, and some mistakes that are caught by my editor. I will comment them here, and might add more in the future.

1) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L365 seems to supersede https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L363. Additionally, https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L372 seems to supersede https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L364.

2) I'm not sure how the entire end functionality works. The line https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L915 seems to find the last children of the last toplevel block, which is exactly two levels. If there are more than two levels, this code won't work correctly.

3) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L1019 misses the argument for refreshCM.

4) (Nit) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L418

handleCopyCut has an inconsistent return (it doesn't seem to expect anything to return, but there's a place with return false). If you meant to stop event bubbling, I don't think it will do that (at least not in non-jQuery framework, which we didn't use).

5) (Nit) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L633

findNodeFromEl returns undefined when the conditional is false. It can also return false from the short-circuiting. This violates the spec that it either returns ASTNode or null

6) (Nit) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L731

handleTopLevelEntry has the same inconsistent return.

7) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L1024

event is unbound

8) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/ast.js#L271

This returns empty string rather than false.

9) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/blocks.js#L255

markText is unused.

10) https://github.com/bootstrapworld/codemirror-blocks/blob/master/src/ast.js#L25

pathIsIndependentOfChangePath is unused

---

In general, I wish that we didn't perform action based on DOM node properties (e.g., reading aria-expanded to see if nodes are expanded or not) or document.activeElement. The principle of big-bang/react is the same: actions should be based on state/world, and DOM node properties are simply presentations. This also means if we want to change presentation in the future, functionalities won't be broken, whereas with the current approach, we need to rewrite a lot of code. A part of reactify work is to clean these up.

[schanzer]

Thanks for putting this together. I've pushed a commit that addresses some of these, and written explanations for others below:

1) For perf reasons, CM hides DOM nodes when they're past the viewport. Calling getBoundingClientRect the first time winds up getting an approximate location, but until the DOM node is added back to the tree this will be inaccurate. That's why we call twice. [UPDATE: fixed in 119cb88e8b30ee17548201be2283002e8561f947]

2) Definitely a bug. Oy. It used to be that [...node] would return every child, which would make this work correctly. I changed the behavior of the iterator long ago, and never had tests to catch this, and it's been broken ever since. [UPDATE: fixed in 48e7fd5b7db2f67cb48aa1ce252da887b7f58da0]

3) Yep. Bug. Fixed.

4, 5, 6, 7, 8) Fixed.

9) markText is a CodeMirror API which we happen to implement, so it's for external-use only. This allows, for example, editors that already use CM to swap in our block editor and still get the syntax highlighting they know and love. Try running this WeScheme example, then turn on block mode and run it again.

10) It's used inside patch()

~11) I used to store expanded state in the AST, and render it via React. This was agonizingly slow, relative to using the native querySelector functionality. I'm hopeful that the Reactify branch will bring back the performance to do it Right(tm). Added a comment about this.

[sorawee]

For perf reasons, CM hides DOM nodes when they're past the viewport. Calling getBoundingClientRect the first time winds up getting an approximate location, but until the DOM node is added back to the tree this will be inaccurate. That's why we call twice.

In that case, we only call it for side-effect, right? Can we avoid the var {top, bottom, left, right} = part? They are unused variables as I understand (because later they are overwritten by the real invocation).