search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Node.length? #471

Closed mindplay-dk closed 8 years ago

mindplay-dk commented 8 years ago

What's this?

https://github.com/guardian/scribe/blob/master/src/plugins/core/inline-elements-mode.js#L12

Nodes do not have a .length property, so most likely that part of the expression simply evaluates as false in every case.

What was this part of the expression intended to accomplish?

rrees commented 8 years ago

My guess would be that this is meant to be a check on the childNodes length.

@OliverJAsh I don't suppose you can recall what this condition was aiming to do can you?

OliverJAsh commented 8 years ago

I don't remember writing that! Sorry that's not very helpful…

mindplay-dk commented 8 years ago

Well, the comment says "If the node is a non-empty element or has content", so that explains the intent. Except this wouldn't work, and one wonders if it should work that way or not?

I'd suggest de-obfuscating that curious use of Array.indexOf and the binary ~ operator as well.

So probably:

        if (treeWalker.currentNode.nodeName === "BR" || treeWalker.currentNode.children > 0) {
          return true; // If the node is a non-empty element or has content
        }

(probably this function should be given a name though, and moved into node.js with the other helpers, and have a test.)