Remove @inject from subject viewers #2719

eatyourgreens · 2022-01-31T11:29:28Z

Remove @inject from BarChartViewer, LightCurveViewer and SubjectGroupViewer. Convert each from a class to a functional component with hooks. I've added a useJSONData hook to load JSON data for BCV and LCV subjects.

The enzyme tests were written to test implementation details, like internal class state or lifecycle methods. I've rewritten them to test the rendered subject viewer wrapper, and its props, either onError or onReady.

Test projects:

Package:
lib-classifier

Towards #2712.

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 · 2022-01-31T17:24:54Z

...sifier/components/SubjectViewer/components/SubjectGroupViewer/SubjectGroupViewerContainer.js

+export function SubjectGroupViewerContainer({
+  addAnnotation,
+  annotation,
+  cellWidth,
+  cellHeight,
+  cellStyle,
+  gridColumns,
+  gridRows,
+  gridMaxWidth,
+  gridMaxHeight,
+  ImageObject = window.Image,
+  interactionMode = 'annotate',
+  isCurrentTaskValidForAnnotation,
+  loadingState = asyncStates.initialized,
+  onError = () => true,
+  onKeyDown = () => true,
+  onReady = () => true,
+  setOnPan = () => true,
+  setOnZoom = () => true,
+  subject = undefined
+}) {


The subject group viewer has a lot of props!

eatyourgreens · 2022-02-04T09:08:22Z

...lassifier/components/SubjectViewer/components/BarChartViewer/BarChartViewerContainer.spec.js

    let nockScope
    let wrapper
    before(function () {
      sinon.stub(console, 'error')
-      cdmSpy = sinon.spy(BarChartViewerContainer.prototype, 'componentDidMount')
-      onErrorSpy = sinon.spy()
      nockScope = nock('http://localhost:8080')


In the project app tests, we mock the Panoptes API with nock. Maybe that would be better for the subject viewer tests too?

eatyourgreens · 2022-02-04T09:09:26Z

...lassifier/components/SubjectViewer/components/BarChartViewer/BarChartViewerContainer.spec.js

    let wrapper

    before(function () {
-      cdmSpy = sinon.spy(BarChartViewerContainer.prototype, 'componentDidMount')
-      cduSpy = sinon.spy(BarChartViewerContainer.prototype, 'componentDidUpdate')
      nockScope = nock('http://localhost:8080')


Same comment about mocking panoptes-staging.zooniverse.org or panoptes.zooniverse.org here.

...lassifier/components/SubjectViewer/components/BarChartViewer/BarChartViewerContainer.spec.js

shaunanoordin · 2022-02-11T15:51:05Z

...Classifier/components/SubjectViewer/components/LightCurveViewer/LightCurveViewerContainer.js

 import { zip } from 'lodash'
 import PropTypes from 'prop-types'
-import React, { Component } from 'react'
-import request from 'superagent'


Goodbye, superagent!

I was going to ask if we shouldn't also remove this from package.json, then I realised that DataImageViwer(?), SingleTextViewer(?), ScatterPlotViewer, and VariableStarViewer still uses the dependency, dangit. I didn't even realise those first two viewers existed.

SingleTextViewer inspired me to write this PR, when I saw that it was still using outdated code from when we first started the monorepo.

shaunanoordin · 2022-02-11T16:11:30Z

packages/lib-classifier/src/helpers/useJSONData/README.md

@@ -0,0 +1,17 @@
+# useJSONData
+
+A custom hook which loads JSON data for a Panoptes subject.


[minor] While this is true, the description is missing one important detail: useJSONData loads JSON data for a Subject, and assumes that Subject that has exactly one JSON(-ish) URL data.

Specifically:

A text file "technically" counts as a JSON URL (hence, JSON-ish ) for this function, because the Panoptes CLI has (had?) issues differentiating JSON/TXT MIME types.

If a Subject has multiple JSON(-ish) URLs, only the first is pulled by useJSONData()

This doesn't matter in current (2018 to 2022) LightCurveViewer (or BarChartViewer, I guess) scenarios, since project owners who use these Viewers know the precise Subject format required. However, I can imagine an edge case where useJSONData doesn't work as described, e.g. when we have a Subject that looks like...

subjectWithTextAndJSON.locations = { 'text/plain': 'actual-boring-plain-text.txt', 'application/json': 'json-object-for-data-viewer.json' } // In this case, the function attempts to load the plain text as JSON, fails at response.json(), and then skips the actual JSON object file.

I'm marking this as a minor thing right now (⭐ I think editing the README text to include the caveats above should suffice), because if a future theoretical SuperDataViewer needs to use this function to get a Subject with 10 JSON files and a dozen TXT files, we can adjust the code accordingly when it's less theoretical.

Really good point, but I think this can wait until we have real examples of subjects, rather than try to second guess what they might look like now.

shaunanoordin · 2022-02-11T16:13:28Z

Running manual tests now for SubjectGroupViewer and LightCurveViewer, brb

shaunanoordin · 2022-02-11T16:58:49Z

Quick Review

I've encountered a problem with the LightCurveViewer, but I'm not sure what's the cause. I seem to be able to replicate it consistently.

Issue 1: when switching between Annotate and Move modes on the LCV, the zoom level resets (❗)

Update: Solved

fem-pr2719-20220211-1646-issue-1-v2.mov

(EDIT: changed to a cleaner video example.) In the video above, I'm zooming in (on a time span between Day 18 and 19), but when I try to make an annotation, the zoom takes me back to the default zoom (the time span of Day 0 to 30)

Testing Setup:

Test URL: https://local.zooniverse.org:8080/?env=production&project=nora-dot-eisner/planet-hunters-tess&demo=true (TESS Planet Hunters)
Baseline: https://www.zooniverse.org/projects/nora-dot-eisner/planet-hunters-tess/classify/workflow/11235?demo=true (issue not detected on live)

Testing steps:

Open the Classifier. You'll start in 'Annotate mode' (the pointing arrow icon)
Click on the 'Move mode' button (four-direction arrow icon) on the Toolbar
Use the mouse to zoom in (mouse wheel) and scroll to (mouse drag) look at an interesting position X on the Subject.
Click on the 'Annotation mode' button on the Toolbar
- Expected: you should STILL be zoomed in on the Interesting Position X on the Subject, so you can place an annotation mark precisely there
- Actual: the zoom level gets reset to 0

Notes:

There are small scenarios where this bug doesn't get triggered, but I haven't been able to fully document those cases yet. e.g. if you use keyboard (left/right, +/-) in Annotation mode to navigate, instead of switching to Move mode and using the mouse to navigate, then you might not notice the problem at all.
I'll also need to double check on master to see if I'm missing something (UPDATE: no issues on master, commit b236963)

~~Status: major bug ❗~~

Update: Solved

Testing ongoing to pick up anything other issues before I circle around to see if I can figure out wtf is going on here. One thing to note is that the D3.js has its own "internal memory" for zoom/pan values (IIRC), so something is making the Viewer think it's refreshing and thus triggering the D3.js zoom reset.

eatyourgreens · 2022-02-11T17:01:07Z

Isn't move mode controlled by a prop? Changing a component prop would certainly trigger a render, although I'm not sure why it would reset zoom.

eatyourgreens · 2022-02-11T17:16:08Z

@shaunanoordin this code generates a new dataPoints array from JSONdata on each render. The LCV is set up to reset the chart if dataPoints changes. That might explain the bug.

shaunanoordin · 2022-02-11T17:19:26Z

Ah, here we go! I see a potential lead.

When switching between Annotation mode and Move mode (or vice versa), this triggers LightCurveViewer.componentDidUpdate()
There's a check in that function for if (!sameSubject) {}
If that triggers, clearChart() is called, which resets the zoom. (i.e. yo you just received a new Subject, please reset the zoom)

I'm checking that !sameSubjcet condition now

shaunanoordin · 2022-02-11T17:21:09Z

Ha! OK, looks like we've come to the same conclusion 👍 Should be an easy fix, we can just pass in the Subject ID to the LCV and use that to trigger sameSubject?

eatyourgreens · 2022-02-11T17:22:25Z

@shaunanoordin already done! See the latest commit.

shaunanoordin · 2022-02-11T17:22:50Z

I see your changes, Jim! 👍 I'll run tests now, brb!

shaunanoordin · 2022-02-11T17:30:42Z

Quick Review (Update)

LightCurveViewer is looking good! 👍

Issue 1 has been solved
I can view a valid LCV Subject
I can switch to Move mode to navigate with a mouse (pan with mouse drag, zoom with mouse scroll)
I can switch to Annotate mode to place an annotation mark using a mouse (mouse drag).
- In Annotate mode, I can move (mouse drag), edit (mouse drag edges of marks), and delete (click on 'X' button) existing marks
I can navigate the LCV using keyboard keys (left/right keys to pan, +/- keys to zoom)
I can zoom in/out on the LCV using the Image Toolbar's zoom in/out buttons.
I can submit Classifications
- When I submit a Classification, the zoom levels reset when the next Subject loads.

(Cripes, I need to turn those 👆 into actual component tests)

shaunanoordin · 2022-02-11T17:58:14Z

Quick Review (Update)

OK, SubjectGroupViewer (Grid Tool) now!

Issue 2: zoom and pan (keyboard only) don't work on the Subject Group Viewer

The problem:

When I try to zoom in, I can only zoom in "one step" at most. (sometimes like from 1.0x to 1.1x, and that's it)
When I try to pan (using keyboard), I can only pan "one step" at most (I can go x-1 y+0 ; OR x+0 y-1 ; OR x+1 y+0 ; OR x+0 y+1. Literally only four possible specific pan values!)
- No issues with drag-to-move, surprisingly.

Analysis:

It seems that for the actions listed above, the zoom/pan levels RESET to default, then apply the changes.
e.g. when zooming in, every 'zoom' action first RESETS the zoom to 1.0, then adds the zoom in step of +0.1, thus locking it at 1.1
e.g. when panning to the right, every 'right key' press first RESETS the pan to x+0 y+0, then adds x+1, thus locking it to x+1 y+0

Test URL: https://local.zooniverse.org:8080/?env=production&project=zookeeper/galaxy-zoo-weird-and-wonderful&demo=true
Baseline: https://www.zooniverse.org/projects/zookeeper/galaxy-zoo-weird-and-wonderful/classify/workflow/18510 (no issues on live)

Tested on/with localhost, macOS, Chrome 98, lib-classifier.

I'll do a quick check on the code to see what's up, but I may need to follow up the debug on Monday

Additional Notes

Full functionality checklist for Subject Group Viewer:

✔️ I can view a Subject consisting of a grid images
- ✔️ The grid is configured as per the workflow (4x4 cells, in this case)
✔️ In Annotation mode, if there's a SubjectGroupComparisonTask active, I can click on grid cells to 'select' images.
- ✔️ I can submit Classifications with the selected images
In Move mode, I should be able to use the mouse to navigate the Subject
- ✔️ I can pan the subject using click and drag
- ❌ I should be able to zoom in/out using the mouse wheel
Using the keyboard, I should be able to navigate the Subject (all images should pan & zoom simultaneously)
- ❌ I should be able to pan the subject using up/down/left/right keys
- ❌ I should be able to zoom in/out using the +/- keys
❌ Using the Image Tool bar's buttons, I should be able to zoom in/out

shaunanoordin · 2022-02-11T18:14:20Z

Yup, I can see that for SubjectGroupViewerContainer, the following state values:

const [panX, setPanX] = useState(0)
const [panY, setPanY] = useState(0)
const [zoom, setZoom] = useState(1)

...keep resetting to their default values for some reason. I haven't been able to trace why, unfortunately. This is now a Monday problem!

eatyourgreens · 2022-02-11T19:05:13Z

I think this is what’s happening:
https://beta.reactjs.org/apis/usestate#updating-state-based-on-the-previous-state

Remove `@inject` from `BarChartViewer`, `LightCurveViewer` and `SubjectGroupViewer`. Convert each from a class to a functional component with hooks.

Try to test rendered output, either `onError` or `onReady`.

Move common data fetching code into a custom hook for JSON data subjects.

Pass `subjectID` so that we can test whether the subject has changed.

Use callbacks to update state from previous state.

eatyourgreens · 2022-02-11T21:26:39Z

@shaunanoordin I've fixed those problems by using callbacks when state is changing continuously.

shaunanoordin · 2022-02-14T18:53:22Z

I think this is what’s happening:
https://beta.reactjs.org/apis/usestate#updating-state-based-on-the-previous-state

Oh, that's new to me! I've never seen setVal(val + 1) with a function passed in, like setVal(_val => _val + 1).

Thanks Jim! I'll wrap up testing later tonight

shaunanoordin

PR Review

Package: lib-classifier

This PR is primarily a refactor/code cleanup for the following Subject Viewer: BarChartViewer, LightCurveViewer, and SubjectGroupViewer (aka Grid Tool)

The container components have been rewritten as functional component.
Notable code changes:
- State-changing hooks that increment existing state values use this format: setValue(_val => _val + 1) and not this: setValue(val + 1)
- LightCurveViewer now checks for Subject IDs to determine if it's looking at the same Subject.

Code changes look good, and testing for both LightCurveViewer and Grid Tool look great. I can't seem to find a live project that uses BarChartViewer, so I have to rely on Storybook for that.

Testing

Tested on localhost with various projects, with macOS + Chrome.

Testing for LightCurveViewer

Test URL: https://local.zooniverse.org:8080/?env=production&project=nora-dot-eisner/planet-hunters-tess&demo=true (TESS Planet Hunters)
✅ Full functionality checklist noted in a previous comment

Testing for SubjectGroupViewer

Test URL: https://local.zooniverse.org:8080/?env=production&project=zookeeper/galaxy-zoo-weird-and-wonderful&demo=true (Galaxy Zoo Weird & Wonderful)
✅ Full functionality checklist noted in a previous comment

Testing for BarChartViewer

Tested on Storybook
✅ Functionality checklist:
- I'm able view the Subject displayed as a bar chart
- I'm able to hover my mouse across different bars to see their numerical values.
- I'm able to tab-navigate across different bars (and see their numerical values when I have focus)

Status

LGTM 👍

eatyourgreens added the refactor Refactoring existing code label Jan 31, 2022

eatyourgreens requested a review from shaunanoordin January 31, 2022 11:29

eatyourgreens force-pushed the remove-inject-subject-viewers branch 9 times, most recently from bf7ea84 to d86303d Compare January 31, 2022 17:17

eatyourgreens commented Jan 31, 2022

View reviewed changes

eatyourgreens requested a review from mcbouslog January 31, 2022 17:29

eatyourgreens force-pushed the remove-inject-subject-viewers branch from 9473cc7 to 64079f2 Compare January 31, 2022 22:58

This was referenced Feb 3, 2022

Bump superagent from 5.1.4 to 7.1.1 #2765

Merged

app-project: Refactor ClassifyPage components to use next-i18next #2784

Merged

eatyourgreens commented Feb 4, 2022

View reviewed changes

...lassifier/components/SubjectViewer/components/BarChartViewer/BarChartViewerContainer.spec.js Outdated Show resolved Hide resolved

eatyourgreens commented Feb 4, 2022

View reviewed changes

...lassifier/components/SubjectViewer/components/BarChartViewer/BarChartViewerContainer.spec.js Outdated Show resolved Hide resolved

shaunanoordin reviewed Feb 11, 2022

View reviewed changes

eatyourgreens force-pushed the remove-inject-subject-viewers branch from d58a917 to 6eaa055 Compare February 11, 2022 15:54

shaunanoordin reviewed Feb 11, 2022

View reviewed changes

eatyourgreens force-pushed the remove-inject-subject-viewers branch 2 times, most recently from 6d7f5f2 to 93e8697 Compare February 11, 2022 16:49

eatyourgreens force-pushed the remove-inject-subject-viewers branch from 38d6cf1 to fc53912 Compare February 11, 2022 17:23

eatyourgreens added 6 commits February 11, 2022 21:26

Remove @Inject from subject viewers

14adc2a

Remove `@inject` from `BarChartViewer`, `LightCurveViewer` and `SubjectGroupViewer`. Convert each from a class to a functional component with hooks.

update tests

cb22e74

Try to test rendered output, either `onError` or `onReady`.

Add useJSONData hook

95d99b3

Move common data fetching code into a custom hook for JSON data subjects.

Update test wording

ac5ca46

LCV: fix the sameSubject test

f10af6d

Pass `subjectID` so that we can test whether the subject has changed.

Fix SGV event handlers

82a7af5

Use callbacks to update state from previous state.

eatyourgreens force-pushed the remove-inject-subject-viewers branch from fae9e2f to 82a7af5 Compare February 11, 2022 21:26

shaunanoordin approved these changes Feb 15, 2022

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Feb 15, 2022

Merge branch 'master' into remove-inject-subject-viewers

3561c44

eatyourgreens merged commit 4b0aeb4 into master Feb 15, 2022

eatyourgreens deleted the remove-inject-subject-viewers branch February 15, 2022 02:37

mcbouslog mentioned this pull request Feb 21, 2022

Light curve subject data not loading #2871

Closed

shaunanoordin mentioned this pull request Feb 22, 2022

Light Curve Viewer: stop Animation when new Subject loads #2882

Merged

eatyourgreens mentioned this pull request Mar 4, 2022

SingleTextViewer: remove implementation tests #2923

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove @inject from subject viewers #2719

Remove @inject from subject viewers #2719

eatyourgreens commented Jan 31, 2022 •

edited

Loading

eatyourgreens Jan 31, 2022

eatyourgreens Feb 4, 2022

eatyourgreens Feb 4, 2022

shaunanoordin Feb 11, 2022

eatyourgreens Feb 11, 2022 •

edited

Loading

shaunanoordin Feb 11, 2022

eatyourgreens Feb 11, 2022

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022 •

edited

Loading

eatyourgreens commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022 •

edited

Loading

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 14, 2022

shaunanoordin left a comment

		@@ -0,0 +1,17 @@
		# useJSONData

		A custom hook which loads JSON data for a Panoptes subject.

Remove @inject from subject viewers #2719

Remove @inject from subject viewers #2719

Conversation

eatyourgreens commented Jan 31, 2022 • edited Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens Jan 31, 2022

Choose a reason for hiding this comment

eatyourgreens Feb 4, 2022

Choose a reason for hiding this comment

eatyourgreens Feb 4, 2022

Choose a reason for hiding this comment

shaunanoordin Feb 11, 2022

Choose a reason for hiding this comment

eatyourgreens Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

shaunanoordin Feb 11, 2022

Choose a reason for hiding this comment

eatyourgreens Feb 11, 2022

Choose a reason for hiding this comment

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022 • edited Loading

Quick Review

eatyourgreens commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022

shaunanoordin commented Feb 11, 2022 • edited Loading

Quick Review (Update)

shaunanoordin commented Feb 11, 2022

Quick Review (Update)

Additional Notes

shaunanoordin commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

eatyourgreens commented Feb 11, 2022

shaunanoordin commented Feb 14, 2022

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review

Testing

Status

eatyourgreens commented Jan 31, 2022 •

edited

Loading

eatyourgreens Feb 11, 2022 •

edited

Loading

shaunanoordin commented Feb 11, 2022 •

edited

Loading

shaunanoordin commented Feb 11, 2022 •

edited

Loading