Fix previous marks from caesar render for transcription task #2001

srallen · 2021-01-22T23:10:38Z

Please request review from @zooniverse/frontend team. If PR is related to design, please request review from @beckyrother in addition.

Package: lib-classifier

Toward #1978, fixes the previous marks render described in this comment: #1978 (comment)

You can test by running the dev classifier with https://localhost:8080/?project=11300&env=production

The previous transcription marks loaded from the caesar reductions are now viewable throughout the workflow tasks and this enables us to answer the question from the second step question task:

The hide previous marks button is always available too:

Compare this to another project with a drawing task (https://localhost:8080/?project=908&workflow=3370), the hide previous marks button is only available on the step with drawing tasks.

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?

…isplaying transcribed lines and the hide marks button.

eatyourgreens · 2021-01-25T09:24:17Z

There's a minor styling bug introduced with this PR. On a drawing task with marks from a previous step, the previous marks show the hand cursor on hover. They can't be selected, so it's only the styling that's broken.
https://localhost:8080/?project=908&workflow=3370

eatyourgreens · 2021-01-25T09:31:55Z

The transcription project looks good to me. Transcribed lines are visible are visible as per the screenshot above. Should I expect any of the purple and blue lines to turn grey?

eatyourgreens · 2021-01-25T09:38:29Z

Hide/Show previous marks is broken on a workflow with two drawing steps.

Hide previous marks on the second step.
Press Back.
Previous marks are rendered again for the first step.
Press Next.
Previous marks are hidden again, and can be revealed with the Show Previous Marks button.

I can reproduce that behaviour on master, so it isn't a blocker here.

eatyourgreens

Code changes look good to me. There's a couple of places where I thought the tests were hard to understand. There's a styling bug introduced here for previous marks from drawing tasks. I'm not sure if you want to fix that here, or open a separate issue?

The styling bug shouldn't be a blocker for the transcription project beta test.

...Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayerContainer.js

...nts/SubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.spec.js

...ctViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarksConnector.spec.js

packages/lib-classifier/src/store/WorkflowStepStore.js

…ubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.spec.js Co-authored-by: Jim O'Donnell <[email protected]>

srallen · 2021-01-25T15:08:25Z

Should I expect any of the purple and blue lines to turn grey?

Not yet, the grey state is the retired state from Caesar. We need more classifications to get them to turn grey.

goplayoutside3 · 2024-02-15T04:30:11Z

...mponents/SubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.js

+        {interactionTaskAnnotations.map((annotation) => {
+          const annotationValuesPerFrame = annotation.value.filter(value => value.frame === frame)


@eatyourgreens this is a long-ago PR, but I have a question particularly about this line introduced here, which is still used in production. annotationValuesPerFrame is intended to filter marks based on the frame passed to PreviousMarks component. However, Travis and I have found that's not always the case and are unsure how long the following bug has been around ~

On projects with multi-frame subjects and more than one workflow step, previous annotations do not filter correctly when switching between frames. For instance, How Did We Get Here is a transcription project with drawing as the first step and a yes/no as the second step. If I draw marks on all the frames in the first step, then go to the 2nd step, navigating between frames renders marks on only one frame. I think the expected behavior is that previous marks should always render on their respective frames. Any thoughts on how to fix this filter?

(More dicussion here).

I can confirm the bug. Here, I've added two lines to frame 0 and one line to frame 1. classification.annotations only contains one line ID for task T0, not three.

Are annotations being lost on live projects that transcribe more than one page per subject?

When I go back to the drawing step, my drawn lines are there. They haven't been added to the classification. So my thought would be that the bug is in the code that adds mark IDs to the current classification.

If I then add a third mark to frame 0, classification.annotations updates to show the three lines from that frame, but loses the line from frame 1.

Based on that, I would say that previousAnnotations is working as expected, but the classification isn't being updated correctly when you draw on more than one frame of a subject. classification.annotations only contains mark IDs for the active frame. I'd expect it to contain the mark IDs for all the transcription lines that I've made.

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayer.js

Lines 107 to 108 in 6b421ed

const markIDs = marks.map((mark) => mark.id)

annotation.update([...markIDs, mark.id])

Since this bug is in production, and there are projects with subjects that have more than one page, I would definitely double check that classifications are being recorded properly when a volunteer adds lines to more than one page.

I've suggested a fix for the markIDs array in #5919.

Here is the bug, by the way. marks here is filtered by frame before being added to the classification in InteractionLayer.

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/InteractionLayerContainer.js

Line 92 in 6b421ed

marks={visibleMarksPerFrame}

#5923 is a more robust fix, replacing the manual update with an autorun reaction that runs whenever the task’s marks change.

Sarah Allen added 2 commits January 22, 2021 15:20

Refactor to use transcription task regardless if in active step for d…

dda0d20

…isplaying transcribed lines and the hide marks button.

Add tests for new store computed views

c2b1614

srallen added the bug Something isn't working label Jan 22, 2021

srallen added this to the IMLS text transcription milestone Jan 22, 2021

srallen mentioned this pull request Jan 22, 2021

Transcription Task Beta Feedback #1978

Closed

8 tasks

Merge branch 'master' into fix-previous-marks-render

c6bec71

eatyourgreens self-assigned this Jan 24, 2021

eatyourgreens approved these changes Jan 25, 2021

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Jan 25, 2021

Update packages/lib-classifier/src/components/Classifier/components/S…

d4cb613

…ubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.spec.js Co-authored-by: Jim O'Donnell <[email protected]>

Clarify test. Remove unnecessary Object.values and check for length.

5584bb8

srallen merged commit 99060e2 into master Jan 25, 2021

srallen deleted the fix-previous-marks-render branch January 25, 2021 15:34

This was referenced Jan 25, 2021

RFC: Hide previous marks behavior with more than one drawing task #2002

Closed

Previous marks block drawing in subsequent workflow steps #2003

Closed

Styling bug with interaction layer #2005

Closed

srallen mentioned this pull request Feb 2, 2021

Refactor PreviousAnnotations and associated utility/computed view functions #2013

Merged

18 tasks

goplayoutside3 reviewed Feb 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix previous marks from caesar render for transcription task #2001

Fix previous marks from caesar render for transcription task #2001

srallen commented Jan 22, 2021

eatyourgreens commented Jan 25, 2021 •

edited

Loading

eatyourgreens commented Jan 25, 2021

eatyourgreens commented Jan 25, 2021

eatyourgreens left a comment

srallen commented Jan 25, 2021

goplayoutside3 Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024

eatyourgreens Feb 15, 2024

eatyourgreens Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024

eatyourgreens Feb 15, 2024

eatyourgreens Feb 15, 2024 •

edited

Loading

		{interactionTaskAnnotations.map((annotation) => {
		const annotationValuesPerFrame = annotation.value.filter(value => value.frame === frame)

	const markIDs = marks.map((mark) => mark.id)
	annotation.update([...markIDs, mark.id])

Fix previous marks from caesar render for transcription task #2001

Fix previous marks from caesar render for transcription task #2001

Conversation

srallen commented Jan 22, 2021

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens commented Jan 25, 2021 • edited Loading

eatyourgreens commented Jan 25, 2021

eatyourgreens commented Jan 25, 2021

eatyourgreens left a comment

Choose a reason for hiding this comment

srallen commented Jan 25, 2021

goplayoutside3 Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

eatyourgreens commented Jan 25, 2021 •

edited

Loading

goplayoutside3 Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024 •

edited

Loading

eatyourgreens Feb 15, 2024 •

edited

Loading