search

guardian / scribe-plugin-noting

9 stars 3 forks source link

Fix VFocus prev() #128

Closed robinedman closed 9 years ago

robinedman commented 9 years ago

Fixes #127

The algorithm used wasn't quite correct. In some cases (such as #127) it'd skip nodes. Amended the test for prev() so it tests this case.

How to test

All unit and integration tests within the noting plugin pass (locally at least) so should be relatively safe. It'd still be worth doing a bit of regression testing to see that notes/flags/corrects works as expected.

rrees commented 9 years ago

The build system is flagging a lodash compatibility problem which is probably due to an upgrade to 3.5.0 in Scribe.

rrees commented 9 years ago

:+1: The code looks like a simplification so that's good

jamespamplin commented 9 years ago

:+1: code looks good

robinedman commented 9 years ago

Good catch @rrees ! It's unfortunately not as simple as bumping the lodash version in this plugin. I don't want to inflate this card with the work necessary to upgrade lodash (as well as Scribe in Composer since I saw we're not using the latest version). So I've run the tests against the version of Scribe that Composer uses at the moment here: https://github.com/guardian/scribe-plugin-noting/pull/130

Since those all pass I'm merging this. I've created https://github.com/guardian/scribe-plugin-noting/issues/131 for upgrading lodash in this plugin