Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Projects: Add subject index pages #1875

Merged
merged 3 commits into from
Mar 3, 2021
Merged

Projects: Add subject index pages #1875

merged 3 commits into from
Mar 3, 2021

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Nov 2, 2020

Add subject index pages for /classify/workflow/[workflowID]/subject-set/[subjectSetID]/subject/[subjectID] and /classify/workflow/[workflowID]/subject/[subjectID]. Pass the subject ID down to the classifier as a prop.

Package:
app-project

Review Checklist

General

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

Components

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

@eatyourgreens eatyourgreens added the enhancement New feature or request label Nov 2, 2020
@eatyourgreens eatyourgreens requested a review from a team November 2, 2020 14:49
@eatyourgreens
Copy link
Contributor Author

Note that client-side routing with getServerSideProps is broken in production, at the moment, because it makes requests to zooniverse.org/_next/data. See #1826.

@eatyourgreens
Copy link
Contributor Author

We should give pages like these unique titles, so that they can be distinguished in the browser history, or when bookmarked, or in link previews. Returning a title from getServerSideProps, in the page props, would do that but probably requires #1823 first (and support for getServerSideProps in production.)

@eatyourgreens
Copy link
Contributor Author

If a volunteer bookmarks one of these pages, what happens when they come back? Should they get the specified subject, but with an 'Already Seen' banner?

@beckyrother
Copy link

Returning to an already-classified subject would display the Already Seen banner and a modal explaining why they can still look at this subject; same with a retired subject.

The modal has two options: to return to the project home page or to choose another subject from the subject set. Choosing another subject opens the modal to choose another subject within the classify page: https://projects.invisionapp.com/d/main#/console/12924056/440898093/preview

Should be able to reuse the modal from the project home page on the classify pages, but should not include the 'back to workflow selection' button and has different text ('choose a subject' rather than 'choose a subject to get started').

@eatyourgreens
Copy link
Contributor Author

@beckyrother we already have subject banners in the classifier. At the moment, there isn't really a mechanism to show different banners for different projects or workflows.

@eatyourgreens eatyourgreens changed the base branch from master to project-initial-props January 5, 2021 13:45
@beckyrother
Copy link

Wishful thinking I guess. Let's just show those current banners. It would be nice to have a quick way to go back to subject selection like the modal would do, but if that's not possible they'll just have to go back.

@eatyourgreens
Copy link
Contributor Author

We could maybe change the messaging depending on the type of workflow that you're working on. I'm not sure how flexible those banners are.

Add subject index pages for /workflow/[workflowID]/subjectSet/[subjectSetID]/subject/[subjectID] and /workflow/[workflowID]/subject/[subjectID]. Pass the subject ID down to the classifier as a prop.
Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Package: app-project
Context: AFAIU, the Engaging Crowds project (2020-2021) is introducing sequential Subjects, and this is part of the larger solution.

This PR adds new possible paths for the front end. Specifically, these paths allow us to specify specific Subjects IDs for the Classifier to serve.

This PR is part of a larger solution. The SubjectID is being passed down to the Classifier, but the Classifier doesn't do anything with it - yet.

Testing

Basic path testing on mac OS 10 + Chrome 88. Opening the example paths listed above lead to the Classifier and not a 404, which is all I'm looking for. No other paths are affected.

Status

LGTM 👍

@github-actions github-actions bot added the approved This PR is approved for merging label Mar 2, 2021
@eatyourgreens eatyourgreens merged commit 1253633 into master Mar 3, 2021
@eatyourgreens eatyourgreens deleted the subject-urls branch March 3, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants