Project: Modal workflow selection #1976

eatyourgreens · 2021-01-13T17:19:59Z

A continuation of #1966. This rewrites the workflow selector to allow us to have different behaviour on the project home page or on the project classify page. On the classify page, we'd like workflow selection to interrupt you with a modal dialog that forces you to choose a workflow, and optional subject set, before continuing to classify.

InVision prototype.

Entering a project via the general Classify URL should prompt to choose a workflow:
http://localhost:3000/projects/msalmon/hms-nhs-the-nautical-health-service/classify?env=production

Entering a project via the URL of a workflow that requires a subject set should prompt for subject set selection:
http://localhost:3000/projects/msalmon/hms-nhs-the-nautical-health-service/classify/workflow/16807?env=production

Neither of those pages should open the workflow tutorial in a popup.

Package:
app-project

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 · 2021-01-13T22:43:26Z

Still to do: new tests and stories. Maybe abstract some of the repeated code into new components.

eatyourgreens · 2021-01-15T15:54:09Z

In local testing with the HMS NHS project, I’m seeing an intermittent bug where the classifier opens a tutorial over the subject set picker, on page load. There might need to be some changes to the classifier to make sure it isn’t trying to open modals when there is already a modal open on the Classify page. Having two popups open is going to be particularly confusing in a screen reader.

srallen · 2021-01-15T16:05:41Z

The tutorial load observes and reacts to the workflow loading, so I think its store could be modified to instead observe and react to the subject load?

eatyourgreens · 2021-01-15T16:08:37Z

Thanks! I think that would definitely fix this. The Classify page could also hold off on passing initial props to the classifier until canClassify is true.

beckyrother

I think this works! Grant mentioned it in Slack but reiterating here that the close buttons do still need to be removed. Everything else looks great!

eatyourgreens · 2021-01-15T16:34:18Z

Thanks for reminding me about the Close buttons (#1981.)

shaunanoordin

PR Review

Package: lib-classifier

This PR update shows the Workflow Selection modal on the /classify path, when a project has multiple active Workflows. Also, this update shows the Subject Selection modal on the /classify/workflow/1234 path, when a Workflow has multiple active Subjects.

Unfortunately, I am encountering major issues while testing this PR - @eatyourgreens was this branch recently rebased to reflect changes made in #1963 ? I'm having issues while testing this.

Major Issue: "No Workflow ID" error"

Issue: whenever I try to go to the /classify page of a Project with multiple active workflows (note: no default workflow), I will see the modal for 2 seconds, before the website crashes with the error ReferenceError: No workflow ID available

Testing setup:

Any project with multiple active workflows, on localhost
- e.g.http://localhost:3000/projects/msalmon/hms-nhs-the-nautical-health-service/classify?env=production with 2 active workflows
- note: I used http://localhost:3000/projects/darkeshard/transformers/classify with 2 active workflows. but please note that this project setup will be changing rapidly as I review different PRs, so best test on your own projects.
macOS 10 + Chrome 87

Testing steps:

go to the /classify route of the selected test project
Expected: the Workflow Selection modal should show, and the Classifier should halt all operations until I select my workflow
Actual: the Workflow Selection modal shows for a few seconds. Then, the app crashes with the on-screen error - "Unhandled Runtime Error - Reference Error: No Workflow ID Available"

Analysis:

From a brief debug of the code, it seems that while the classifier correctly identifies when to serve the Workflow Selection modal, the "try to fetch a workflow" logic is STILL going on, and it's trying to fetch Workflow "undefined".
The "trying to fetch workflow undefined" action might explain why there's a few second delay between seeing modal and the app crashing.

Status

Issues detected, so I'm marking this review as a CR for the time being...

...BUT after digging into this further, I realise that the issue might be separate from this PR. After all, this PR adds a new Workflow Selection component (which seems to work great, even though my full testing capability is limited due to the issue) while the issue seems to stem from a workflow selection logic.

Hence, the question: was this PR rebased after 1963? I was looking at my tests from 1963 - specifically Scenario 4 - and I realise that the "No Workflow ID" error was encountered there.

Scenario 4: project has multiple active workflows, none of which is the default

the "Classification" link in the header leads to: /projects/darkeshard/transformers/classify (This is sensible)

navigating directly to the /classify route results in: ERROR, No Workflow ID available (Which IS expected 👍 )

So, thoughts: I think this PR (i.e. add new component) works fine in theory (code looks good), but I can't approve it until I've fully tested it, and that requires a fix to the workflow selection logic, which should bea separate PR.

shaunanoordin · 2021-01-20T00:31:02Z

Footnote: I'm now reviewing 1966 after 1976. I'm not sure if getting 1966 merged first will address the "No Workflow ID" error described in the PR Review, since - at the time of writing - 1966 has conflicts that need resolving.

eatyourgreens · 2021-01-20T17:47:33Z

@shaunanoordin #1966 is the 'quick fix' to get workflow selection onto the Classify page so that we know we can use it in the Engaging Crowds beta. This PR is the more considered approach, using the final design and an interstitial popup, but I kept it seperate just in case it turned out that the classifier breaks when you render it before workflow selection is complete. I think definitely review #1966 first, then have a look at this PR.

So #1966 is essential for the beta test. This is nice-to-have, if we can get it working in time.

eatyourgreens · 2021-01-21T07:11:29Z

Some git rebase --onto magic should fix those merge conflicts.

EDIT: totally worked!

eatyourgreens · 2021-01-21T09:55:33Z

Re. the workflow ID error, we have a design for a placeholder classifier that should show while we’re choosing a workflow, subject set etc. I’ll open an issue for that.

eatyourgreens · 2021-01-21T12:02:02Z

That error you're seeing is being thrown by selectWorkflow here, because we've mounted the classifier without a workflow, and we don't select a default workflow for you now. You have to make a choice from the menu.

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

Line 68 in 9300aad

this.classifierStore.workflows.selectWorkflow(workflowID, subjectSetID)

eatyourgreens · 2021-01-25T11:10:00Z

@shaunanoordin I've fixed the workflow ID errors. This PR is now a must-have for the Engaging Crowds beta test.

mcbouslog

Code LGTM, no blocking comments. Nice separation of components, changes to Classifier.js straightforward. Tests and stories are very helpful.

I see some SubjectSetCard propType warnings (expected ___ received undefined).

Confirming as already noted that /classify/workflow initial load had tutorial modal on top of subject selection modal. Not sure if there's a related issue/PR, but not blocking.

In addition to production project noted in comments, tested from project home and /classify on staging projects projects/nora-dot-test/planet-finders-beta and projects/wgranger-test/anti-slavery-testing, without issues/warnings specific to this PR.

packages/app-project/src/shared/components/SubjectSetPicker/SubjectSetPicker.js

...-project/src/screens/ProjectHomePage/components/Hero/components/WorkflowMenu/WorkflowMenu.js

packages/app-project/src/screens/ClassifyPage/ClassifyPage.js

eatyourgreens · 2021-01-27T09:06:15Z

Thanks for the review @mcbouslog. I'll have a look at fixing the tutorial on this branch but it might need its own PR.

Check for a subject set ID if the selected workflow is grouped. If there's no subject set ID, showing the workflow selector with the subject set picker active.

Move workflow selection state into the workflow selector. Add an onSelect handler to the workflow select buttons.

Wrap the workflow selector in a modal on the classify page. Force the modal to be active until a workflow is selected.

Extract workflow selection state from the workflow selector, so that we can have different behaviour in the Hero component or the project Classify page. Consumers are now expected to provide an onSelect handler, which reacts to workflow selection changes.

Remove the Modal component from the subject set picker. Consumers can now choose how they display it. The Hero component wraps the picker in a new modal. The Classify page wraps the picker in the same modal as the workflow selector.

Move all the shared code for the home page workflow menu into a WorkflowMenu component. Add stories and tests.

Move the subject set picker to @shared/components/SubjectSetPicker.

Add a back button to the subject set picker, and an onClose callback which is called when the button is clicked.

Allow workflow ID and subject set ID to be set after the classifier has mounted.

Use the ZRC PlainButton component. Localise the subject set picker. Remove unused onEsc from the Classify page code.

Changes have been addressed now.

eatyourgreens added the enhancement New feature or request label Jan 13, 2021

eatyourgreens force-pushed the workflow-selection-modal branch from 7d45884 to 0f2241a Compare January 13, 2021 17:46

eatyourgreens force-pushed the workflow-selection-modal branch 4 times, most recently from 0704f21 to c1aa0e7 Compare January 14, 2021 12:26

eatyourgreens marked this pull request as ready for review January 14, 2021 14:16

eatyourgreens requested a review from shaunanoordin January 14, 2021 14:17

eatyourgreens added this to the Engaging Crowds: HMS NHS beta test milestone Jan 14, 2021

eatyourgreens requested a review from beckyrother January 14, 2021 14:30

eatyourgreens assigned shaunanoordin Jan 14, 2021

beckyrother approved these changes Jan 15, 2021

View reviewed changes

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

eatyourgreens mentioned this pull request Jan 15, 2021

ZRC: allow for Modal components that can't be closed #1981

Closed

shaunanoordin previously requested changes Jan 19, 2021

View reviewed changes

eatyourgreens force-pushed the workflow-selection branch from be7f6f4 to 2fdf9b8 Compare January 20, 2021 09:53

eatyourgreens force-pushed the workflow-selection-modal branch from 686cddb to b6143e3 Compare January 20, 2021 10:10

Base automatically changed from workflow-selection to master January 20, 2021 20:06

eatyourgreens force-pushed the workflow-selection-modal branch from b6143e3 to a04f6fe Compare January 21, 2021 08:15

eatyourgreens requested a review from shaunanoordin January 25, 2021 11:07

eatyourgreens force-pushed the workflow-selection-modal branch from ede735e to 8bbb34e Compare January 25, 2021 11:08

mcbouslog self-requested a review January 26, 2021 19:34

mcbouslog approved these changes Jan 26, 2021

View reviewed changes

eatyourgreens added 10 commits January 27, 2021 09:07

Check grouped workflows for a selected subject set

da21200

Check for a subject set ID if the selected workflow is grouped. If there's no subject set ID, showing the workflow selector with the subject set picker active.

Refactor the workflow selector

b0a48c4

Move workflow selection state into the workflow selector. Add an onSelect handler to the workflow select buttons.

Classify page: modal workflow selector

b6a1bb8

Wrap the workflow selector in a modal on the classify page. Force the modal to be active until a workflow is selected.

Extract the modal from the subject set picker

2356cc8

Remove the Modal component from the subject set picker. Consumers can now choose how they display it. The Hero component wraps the picker in a new modal. The Classify page wraps the picker in the same modal as the workflow selector.

Update component stories

c12dc42

Add WorkflowMenu

a94e6e0

Move all the shared code for the home page workflow menu into a WorkflowMenu component. Add stories and tests.

Reorganise files

e2f18e2

Move the subject set picker to @shared/components/SubjectSetPicker.

Add Back To Workflow button

3f9bc56

Add a back button to the subject set picker, and an onClose callback which is called when the button is clicked.

Catch workflow ID errors in the classifier

de1e93a

Allow workflow ID and subject set ID to be set after the classifier has mounted.

eatyourgreens force-pushed the workflow-selection-modal branch from 8bbb34e to de1e93a Compare January 27, 2021 09:07

PR review

9a8c729

Use the ZRC PlainButton component. Localise the subject set picker. Remove unused onEsc from the Classify page code.

eatyourgreens merged commit fbf7256 into master Jan 27, 2021

eatyourgreens deleted the workflow-selection-modal branch January 27, 2021 09:56

This was referenced Mar 2, 2022

Classifier: record tutorials as completed when you click Get Started #2904

Merged

Workflow selection modal showing on PH TESS #2910

Closed

Project: Classify page allows the classifier tutorial to appear when other modals are already open. #2909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project: Modal workflow selection #1976

Project: Modal workflow selection #1976

eatyourgreens commented Jan 13, 2021 •

edited by mcbouslog

Loading

eatyourgreens commented Jan 13, 2021

eatyourgreens commented Jan 15, 2021

srallen commented Jan 15, 2021

eatyourgreens commented Jan 15, 2021

beckyrother left a comment

eatyourgreens commented Jan 15, 2021 •

edited

Loading

shaunanoordin left a comment •

edited

Loading

shaunanoordin commented Jan 20, 2021

eatyourgreens commented Jan 20, 2021 •

edited

Loading

eatyourgreens commented Jan 21, 2021 •

edited

Loading

eatyourgreens commented Jan 21, 2021

eatyourgreens commented Jan 21, 2021

eatyourgreens commented Jan 25, 2021

mcbouslog left a comment

eatyourgreens commented Jan 27, 2021

Project: Modal workflow selection #1976

Project: Modal workflow selection #1976

Conversation

eatyourgreens commented Jan 13, 2021 • edited by mcbouslog Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens commented Jan 13, 2021

eatyourgreens commented Jan 15, 2021

srallen commented Jan 15, 2021

eatyourgreens commented Jan 15, 2021

beckyrother left a comment

Choose a reason for hiding this comment

eatyourgreens commented Jan 15, 2021 • edited Loading

shaunanoordin left a comment • edited Loading

Choose a reason for hiding this comment

PR Review

Major Issue: "No Workflow ID" error"

Status

shaunanoordin commented Jan 20, 2021

eatyourgreens commented Jan 20, 2021 • edited Loading

eatyourgreens commented Jan 21, 2021 • edited Loading

eatyourgreens commented Jan 21, 2021

eatyourgreens commented Jan 21, 2021

eatyourgreens commented Jan 25, 2021

mcbouslog left a comment

Choose a reason for hiding this comment

eatyourgreens commented Jan 27, 2021

eatyourgreens commented Jan 13, 2021 •

edited by mcbouslog

Loading

eatyourgreens commented Jan 15, 2021 •

edited

Loading

shaunanoordin left a comment •

edited

Loading

eatyourgreens commented Jan 20, 2021 •

edited

Loading

eatyourgreens commented Jan 21, 2021 •

edited

Loading