Add activeTool validation on frame change to MultiFrameViewerContainer

[mcbouslog]

Package:

• lib-classifier

Closes #2810.

Describe your changes:

• adds active tool validation on frame change to MultiFrameViewerContainer

Helpful explanations that will make your reviewer happy:

• What Zooniverse project should my reviewer use to review UX?
  • (confirm existing issue) https://www.zooniverse.org/projects/rachaelsking/corresponding-with-quakers/classify/workflow/20543?demo=true
  • (check fixed) https://local.zooniverse.org:8080/?project=rachaelsking/corresponding-with-quakers&workflow=20543&env=production&demo=true
  • (multi frame viewer without active tool to check negative side effects) https://local.zooniverse.org:8080/?project=1876&demo=true
• What user actions should my reviewer step through to review this PR?
  1. on a frame start to create transcription line, but don't click twice to finish the line (leaving line unfinished, one dot)
  2. go to different frame, make test transcription line, everything will look ok
  3. for Corresponding Quakers answer second task Y/N question
  4. on prod - can't click Next vs on local - can click Next
• Which storybook stories should be reviewed? - n/a
• Are there plans for follow up PR’s to further fix this bug or develop this feature? - maybe, this will hopefully fix most of related cases, but user can still be prevented from advancing with an incomplete line as further detailed in this comment in the related issue

Review Checklist

General

• Are the tests passing locally and on Travis?
• Is the documentation up to date?

Components

• Has a storybook story been created or updated?
• Is the component accessible?
  • Can it be used with a screen reader? BBC guide to testing with VoiceOver
  • Can it be used from the keyboard? WebAIM guide to keyboard testing
  • Is it passing accessibility checks in the storybook?

Apps

• Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
• Does it work on mobile?
• Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

• Is the changelog updated?
• Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

• Did the app deploy to https://frontend.preview.zooniverse.org/projects/:project-name/:owner or https://frontend.preview.zooniverse.org/about?
• Is the new feature working or bug now fixed?
  • Is there a Talk or blog post written to announce the new feature(s)?
• Is the design working across browsers (Firefox, Chrome, Edge, Safari) and mobile?
  • Is this approved by our designer?
• Is this ready for production deployment?

[eatyourgreens]

The bug fix looks good. I'm seeing stray marks deleted when I change frame on this branch. If it's quick, and easy, to test that invalid marks are deleted, then I'd ask for a test that checks that behavior, in order to avoid regressions. If not, then lets get this deployed ASAP but do take a look at #2808, which refactors this viewer as a functional component with hooks for subject data fetching.

[eatyourgreens]

Not a blocking comment, but wrapper.instance() should be avoided in tests, since it's null for functional components.

This test doesn't tell us what validation does. If the expectation is that incomplete marks are deleted, then a better test would set up an incomplete translation line and expect it to be deleted on frame change. That would be a behaviour-driven test, whereas this tests how the code is implemented. Implementation can change, in refactors, without changing behaviour, which makes tests like this one brittle.

[mcbouslog]

Yes I agree, I'll refactor this test better asap.

[eatyourgreens]

No hurry to do this right now, by the way.

[mcbouslog]

@eatyourgreens I think this test is better now, not ideal (still using wrapper.instance()), but since #2808 refactors to functional component and relatedly refactors tests with mount then we can include removing wrapper.instance() in that PR? Sorry that #2808 will need to be refactored because of this PR, but I can help or will prioritize review.

[eatyourgreens]

Sounds good. I wouldn’t worry about breaking #2808.

I think the equivalent fix, using hooks, would be something like

useEffect(() => {
 activeTool?.validate()
}, [frame])

[eatyourgreens]

New test looks good. 👍🏻