guardian / scribe-plugin-noting

9 stars 3 forks source link

The name isWithinNote is misleading #54

Closed robinedman closed 9 years ago

robinedman commented 9 years ago

The name isWithinNote doesn't do what the name suggests.

It returns true for:

This function is used when merging adjacent notes. The idea is that you call this function on a focus that you already know is within a note, and then you filter forwards or backwards until you step out of what should be considered part of this note.

Previously the function was called stillWithinNote, but maybe we can come up with a better name.

robinedman commented 9 years ago

To illustrate, the first test will fail if we change the text node in the example to a P:

it('should identify when a focus is not contained within a note', function() {
    var p = h('p', 'This is some text');
    p = new VFocus(p);
    expect(isWithinNote(p)).to.be.false;
});

This is because it returns true for all VNodes, no matter if they're currently within a note or not.

If I read the tests correctly they seem to believe that this function checks whether any given focus is a note or has a note as a parent (which is what !!findParentNote does).

cutandpastey commented 9 years ago

Closing as https://github.com/guardian/scribe-plugin-noting/pull/84 fixed this.