VV - Implement Classifier Tool/Task #6511

kieftrav · 2024-12-03T16:07:32Z

Package

lib-classifier

Describe your changes

Create the Volumetric Tool/Task and properly wire it into the Classifier. Also creates the basic structure for the Annotation, but that will be refactored/fleshed out in the follow up PR. Specifically, there is a missing VolumetricAnnotation.spec.js file that is missing and will be added in the next PR (along with the incorporation of the Volumetric Viewer's annotation model).

How to Review

All tests should pass
Volumetric Task should load in local Storybook
Loading up the Classifier locally with my staging Volumetric Viewer task should load up the TaskArea properly
Design review will be completed in a later PR when Sean's styles are incorporated fully
README should be descriptive and helpful

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

New Feature

The PR creator has listed user actions to use when testing the new feature
Unit tests are included for the new feature
A storybook story has been created or updated
- Example of SlideTutorial component with docgen comments, and its story

Post-merge

This PR adds translations keys to English dictionary(s). See guidance for pulling new keys to Lokalise here.

coveralls · 2024-12-03T16:14:18Z

coverage: 77.58% (+0.04%) from 77.545%
when pulling dfd24ce on vv-tool
into 0a09c9d on master.

goplayoutside3

I can run lib-classifier locally or app-project locally and see a viewer + task at https://local.zooniverse.org:3000/kieftrav/volumetric-viewer 👍

Two blocking things:

I'm unable to run lib-classifier's Storybook on this branch. Every story errors with Error: Cannot find module 'ajv-formats'. Is that the case for you?
The handling of disabled in VolumetricTask is hardcoded and therefore contradicts its Readme statement about disabling the task UI until the subject's json content is loaded.

goplayoutside3 · 2024-12-06T18:01:51Z

...lassifier/src/plugins/tasks/components/TaskInput/components/TaskInputLabel/TaskInputLabel.js

+  align-content: center;
  display: block;


This is display: block not flex, so align-content won't apply. Did you mean to include this change?

So, when I revert this the alignment looks off. My understanding is that the align-content helps with the current element in the context of it's parent flex attribute. Could be wrong, but was trying to fix the visual issue you see in the screenshots below. Happy to revert if it should be handled in another way.

Ah I see. Through PR history it looks like the display: block was copy pasted from when the label text was a span instead of a Grommet Text. This alignment change is fine to keep in this PR, but I recommend removing display: block to avoid confusion during code read. TBH it doesn't even need the margin: 1em 0 either. That was probably an attempt to vertically align the text awhile back, now solved by align-content instead.

Awesome - removed both and looks good.

goplayoutside3 · 2024-12-06T18:11:49Z

packages/lib-classifier/src/plugins/tasks/experimental/volumetric/components/VolumetricTask.js

+
+      <TaskInput
+        checked={true}
+        disabled={false}


The ReadMe for this components says "The volumetric task is disabled until the subject's JSON content is loaded". You've hardcoded disabled as false here. How is the task disabled while JSON content is loading? Did you mean to add that feature in a separate PR?

I updated the README and code... I meant to disable the task until the Subject is loaded. This has been fixed/added and elaborated a bit more in my reply below.

goplayoutside3 · 2024-12-06T18:14:13Z

packages/lib-classifier/src/plugins/tasks/experimental/volumetric/components/VolumetricTask.js

+      </StyledInstructionText>
+
+      <TaskInput
+        checked={true}


Is this hardcoded as true because there's only ever one active tool in any volumetric project? If that's the case please include a brief code comment here because checked and index props for TaskInput were originally designed for multi-tool drawing tasks.

Yes - that's why. Added the comment.

goplayoutside3 · 2024-12-06T18:22:02Z

...s/lib-classifier/src/plugins/tasks/experimental/volumetric/components/VolumetricTask.spec.js

+    it("should show the classification count", function () {
+      render(<DefaultStory />)
+      expect(screen.getByText("InputStatus.drawn")).to.be.ok()
+    })


This works for now, but InputStatus.drawn also includes the variable count. A more robust test would look for count = 3 (because that's what you've hardcoded it as for now), and when the annotations feature is added for volumetric subjects, then you'd pass annotations to the VolumetricTask stories and look for the appropriate count number used in the stories.

Yup - agreed. That'll come in the next PR.

goplayoutside3 · 2024-12-06T18:25:24Z

packages/lib-classifier/src/plugins/tasks/experimental/volumetric/models/VolumetricTask.js

@@ -0,0 +1,23 @@
+import cuid from "cuid"


A comment just for awareness that if you'e going to continue using cuid it is a deprecated third-party package as documented in #6493. All of the other task models still use it so this is just an fyi.

Yup - thank you for the cross-reference visibility

goplayoutside3 · 2024-12-06T18:31:27Z

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

-      return self.experimental_tools.includes('volumetricViewer')
+      return self.experimental_tools.includes('volumetricProject')


I noticed the label on your staging project's admin page is still VolumetricViewer. Are there still updates needed in the PFE codebase to align it with VolumetricProject?

Nope - these will be kept in sync when PFE PR #7227 merges & is deployed.

kieftrav · 2024-12-09T18:10:52Z

I can run lib-classifier locally or app-project locally and see a viewer + task at https://local.zooniverse.org:3000/kieftrav/volumetric-viewer 👍

Two blocking things:

I'm unable to run lib-classifier's Storybook on this branch. Every story errors with Error: Cannot find module 'ajv-formats'. Is that the case for you?

No - I did a yarn panic && yarn bootstrap and it worked just fine to run yarn storybook within the lib-classifier directory... I did a quick search on the ajv-formats on NPM and it looks like URL/date formatting/parsing... Cruft from some other work maybe?

The handling of disabled in VolumetricTask is hardcoded and therefore contradicts its Readme statement about disabling the task UI until the subject's json content is loaded.

Good catch. I added in a check for the subjectReadyState to get the Subject's loading state. This will also be the basis for the Annotation details that'll be pulled into the next PR.

goplayoutside3 · 2024-12-09T23:11:46Z

packages/lib-classifier/src/plugins/tasks/experimental/volumetric/components/VolumetricTask.js

+function VolumetricTask({ task }) {
+  const stores = useContext(MobXProviderContext)
+  const subjectReadyState = stores.classifierStore?.subjectViewer?.loadingState ?? asyncStates.loading


Suggested change

function VolumetricTask({ task }) {

const stores = useContext(MobXProviderContext)

const subjectReadyState = stores.classifierStore?.subjectViewer?.loadingState ?? asyncStates.loading

function VolumetricTask({ disabled = false, task }) {

disabled is already available as a prop from Tasks

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/TaskArea/components/Tasks/Tasks.js

Line 56 in 0a09c9d

disabled={disabled || !ready}

No need to hook into context, you should use the disabled prop already provided. Examples are in SurveyTask, SimpleDropdownTask, TextFromSubjectTask, SingleChoiceTask, etc.

(Sorry if this shows up as a dupe notification, I just wanted to re-comment with code changes suggested)

Got this change incorporated as well. Thanks!

goplayoutside3

Storybook looks good. See my two recent comments for non-blocking code clean up.

kieftrav marked this pull request as draft December 3, 2024 16:07

kieftrav changed the title ~~WIP: VV - ToolModel~~ WIP: VV - Tool & Task Dec 3, 2024

kieftrav force-pushed the vv-tool branch from 99bf9c2 to d4f9d79 Compare December 5, 2024 15:10

kieftrav changed the title ~~WIP: VV - Tool & Task~~ VV - Implement Classifier Tool Dec 5, 2024

kieftrav force-pushed the vv-tool branch from d4f9d79 to ed77165 Compare December 5, 2024 16:58

kieftrav marked this pull request as ready for review December 5, 2024 16:59

kieftrav force-pushed the vv-tool branch from ed77165 to 525d6d1 Compare December 5, 2024 16:59

kieftrav changed the title ~~VV - Implement Classifier Tool~~ VV - Implement Classifier Tool/Task Dec 5, 2024

kieftrav requested a review from goplayoutside3 December 5, 2024 17:06

goplayoutside3 self-assigned this Dec 6, 2024

goplayoutside3 reviewed Dec 6, 2024

View reviewed changes

kieftrav force-pushed the vv-tool branch from 525d6d1 to 1957883 Compare December 9, 2024 18:13

kieftrav requested a review from goplayoutside3 December 9, 2024 18:20

goplayoutside3 reviewed Dec 9, 2024

View reviewed changes

goplayoutside3 approved these changes Dec 9, 2024

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Dec 9, 2024

kieftrav added 3 commits December 10, 2024 08:06

Create Volumetric Task and stub out Annotation

4b4cb4b

PR review fixes.

ad12508

PR revision round 2

dfd24ce

kieftrav force-pushed the vv-tool branch from 1957883 to dfd24ce Compare December 10, 2024 14:19

kieftrav merged commit cd2796e into master Dec 10, 2024
7 checks passed

kieftrav deleted the vv-tool branch December 10, 2024 14:24

goplayoutside3 mentioned this pull request Dec 10, 2024

Classifier: Clean up TaskInput styling #6532

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VV - Implement Classifier Tool/Task #6511

VV - Implement Classifier Tool/Task #6511

kieftrav commented Dec 3, 2024 •

edited

Loading

coveralls commented Dec 3, 2024 •

edited

Loading

goplayoutside3 left a comment

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024 •

edited

Loading

goplayoutside3 Dec 9, 2024

kieftrav Dec 10, 2024

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024 •

edited

Loading

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024

goplayoutside3 Dec 6, 2024

kieftrav Dec 9, 2024

kieftrav commented Dec 9, 2024

goplayoutside3 Dec 9, 2024

kieftrav Dec 10, 2024

goplayoutside3 left a comment

		return self.experimental_tools.includes('volumetricViewer')
		return self.experimental_tools.includes('volumetricProject')

VV - Implement Classifier Tool/Task #6511

VV - Implement Classifier Tool/Task #6511

Conversation

kieftrav commented Dec 3, 2024 • edited Loading

Package

Describe your changes

How to Review

General

General UX

New Feature

Post-merge

coveralls commented Dec 3, 2024 • edited Loading

goplayoutside3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieftrav Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieftrav Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieftrav commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goplayoutside3 left a comment

Choose a reason for hiding this comment

kieftrav commented Dec 3, 2024 •

edited

Loading

coveralls commented Dec 3, 2024 •

edited

Loading

kieftrav Dec 9, 2024 •

edited

Loading

kieftrav Dec 9, 2024 •

edited

Loading