Refactor classifier layout per survey task #6297

mcbouslog · 2024-09-12T20:52:19Z

Package

lib-classifier

Linked Issue and/or Talk Post

closes Survey task: increase task area width to 540px for workflow with survey task #6237 , closes Task area is not responsive for Survey tasks #5093

Describe your changes

add usesSurveyTask view to Workflow store
pass usesSurveyTask from Layout to CurrentLayout
update CenteredLayout, MaxWidth, and NoMaxWidth with larger task area if usesSurveyTask
MaxWidth and NoMaxWidth between 769px-1120px use 9fr 5fr for viewer task areas, these changes include aligning viewer and task vertically at 1119px if usesSurveyTask
center task nav buttons (Done & Talk, Done, Next, Back)

How to Review

Helpful explanations that will make your reviewer happy:

What Zooniverse project should my reviewer use to review UX?
- staging test project with survey task: https://local.zooniverse.org:3000/projects/markb-panoptes/survey-task-test-project/classify/workflow/2434?env=staging
What user actions should my reviewer step through to review this PR?
1. load workflow with survey task
2. note wider task area with survey task
3. change display width and note how layout changes (horizontal vs vertical viewer and task alignment)
Which storybook stories should be reviewed?
Are there plans for follow up PR’s to further fix this bug or develop this feature? see other survey task issues

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

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

Refactoring

The PR creator has described the reason for refactoring
The refactored component(s) continue to work as expected

coveralls · 2024-09-12T20:59:56Z

coverage: 78.512% (-0.06%) from 78.571%
when pulling 86e06fc on lib-classifier_layout-with-survey-task
into db7bd5e on master.

goplayoutside3

A couple of things before approval:

Functions that start with use are reserved for custom React hooks. Because the function to detect if a workflow has a survey task is a store view, it should instead be named hasSurveyTask. I think that'll be just a quick find + replace everywhere that says usesSurveyTask in this PR.
In the survey task area, the choice carousel of images is left-aligned when it should be centered in the task area. This style bug is already present on production and can be seen if viewing the classifier layout on a mobile screen such an ipad. Here's a few screenshots:

seanmiller26 · 2024-09-16T16:56:22Z

@goplayoutside3 Your 2nd concern might be better addressed in a separate PR related to #6239. Thoughts, @mcbouslog?

mcbouslog · 2024-09-17T15:57:48Z

A couple of things before approval:

Functions that start with use are reserved for custom React hooks. Because the function to detect if a workflow has a survey task is a store view, it should instead be named hasSurveyTask. I think that'll be just a quick find + replace everywhere that says usesSurveyTask in this PR.

@goplayoutside3 - the Workflow store also includes usesTranscriptionTask and usesMachineLearnt, I could do a quick find and replace for those as well?

mcbouslog · 2024-09-17T16:11:02Z

In the survey task area, the choice carousel of images is left-aligned when it should be centered in the task area. This style bug is already present on production and can be seen if viewing the classifier layout on a mobile screen such an ipad.

@goplayoutside3 Your 2nd concern might be better addressed in a separate PR related to #6239. Thoughts, @mcbouslog?

@seanmiller26 - yes the Choice image Carousel will be further refactored in #6239 (expanded to full width of choice box), but I will add a interim edit to center align the current image Carousel to fix the current issue that's more noticeable with the larger task area width.

mcbouslog · 2024-09-17T16:13:18Z

@seanmiller26 - I'll deploy this to https://fe-project-branch.preview.zooniverse.org after homepage and stats pages deployment for your review. I'll message you on Slack once it's available.

I think the key question will be widths of 1040px and below, and how the subject and task areas alignment change in workflows with and without a survey task.

seanmiller26 · 2024-09-18T16:21:13Z

@mcbouslog I have some time to review today. Is this ready to preview?

mcbouslog · 2024-09-18T17:57:24Z

@mcbouslog I have some time to review today. Is this ready to preview?

Yes!

staging: https://fe-project-branch.preview.zooniverse.org/projects/markb-panoptes/survey-task-test-project?env=staging
production ( ⚠️ live project ⚠️ ): https://fe-project-branch.preview.zooniverse.org/projects/zooniverse/gravity-spy

seanmiller26 · 2024-09-18T18:34:33Z

I'm not sure exactly the scope of this specific PR, but I have some styling to address.

Padding should be 20px on all sides except the bottom, underneath the Done buttons = 30px.
Padding between the choices and the Done buttons should be 30px. Note: the 'Showing x' and 'Clear Filters' won't be in this area anymore, but that rearrangement will probably in in another PR.

Otherwise the basic adjustment to 540px looks good. And I like where it switches to a vertical layout.

seanmiller26 · 2024-09-18T18:38:35Z

Noticed this behavior right around the 1279-1280px view.

Maybe this is better addressed in the choices PR.

mcbouslog · 2024-09-23T15:49:29Z

Padding should be 20px on all sides except the bottom, underneath the Done buttons = 30px.

Should this apply only to survey tasks or all tasks?

The task area padding is set here: zooniverse/front-end-monorepo/packages/lib-react-components/src/Tabs/theme.js . If the 20px is just for the survey task, I'll connect the workflow store similarly to how it is connected to Layout and conditionally style.

seanmiller26 · 2024-09-23T16:13:20Z

We discussed in person that we'll open a separate PR to address styling across all Tasks. So, no need to address them in this PR.

goplayoutside3

I tested a few projects locally (active projects), and noticed in this PR transcription projects are incorrectly given MaxWidth layout styling. For example, PRINT run locally and PRINT on production. Please double check your changes to the lib-classifier Layout components do not affect non survey task projects.

eatyourgreens · 2024-09-24T16:39:16Z

packages/lib-classifier/src/store/WorkflowStore/Workflow/Workflow.js

+    get hasSurveyTask() {
+      const anySurveyTasks = self.tasks && Object.values(self.tasks).some(task => {
+        return task.type === 'survey'
+      })
+
+      return anySurveyTasks
+    },


If I can make a code design suggestion here, I would add a check for self.hasSurveyTask to workflow.layout, then return a new layout for survey tasks. The advantage of that would be that you avoid accidentally introducing side effects into layouts that are already in use for other projects. For example, that would avoid accidental changes to transcription workflows, when all you want to do is change the layout for camera survey workflows.

That change would also mean that you don't change the API design for layouts in general. At the moment, each existing layout has to handle a new hasSurveyTask argument.

Agreed! That would make much more sense to create a SurveyTaskLayout component rather than impact the existing ones.

Although what would happen if a workflow has a survey task, but also configuration.limit_subject_height set as true 🤔

Oh, that’s a good point. Layout settings can’t be easily combined.

A couple of ideas:

use a boolean attribute/prop to limit the subject height:

<SurveyTaskLayout limitSubjectHeight />

use composition to limit the subject height:

<LimitedHeightLayout> <SurveyTaskLayout /> </LimitedHeightLayout>

A workflow with a survey task could use the default MaxWidth, subject height limited CenteredLayout, or if the workflow tasks also contain a transcription task the NoMaxWidth layout.

The limited height setting causes some weird layout bugs too.

Subject viewer changes size during classification when 'limit subject height' is enabled #6275

Limit subject height shrinks subjects too much on small screens #6061

mcbouslog · 2024-09-25T16:55:12Z

@goplayoutside3 - I fixed the transcription project (NoMaxWidth) issue in 0eeffc9. I tested a lot of the live projects, including transcription, question, and survey task workflows and the layout was consistent or changed as expected if the workflow tasks contain a survey task.

I'm sure the layouts could use additional refactoring as noted, but I'm not sure if we want to further refactor in this branch or iterate on subsequently.

goplayoutside3 · 2024-09-25T17:51:01Z

.../Classifier/components/TaskArea/components/Tasks/components/TaskNavButtons/TaskNavButtons.js

-    <Box direction='row'>
+    <Box
+      direction='row'
+      justify='center'


What project was this needed for? The buttons in this component grow to their container, so why is justify added?

I think this is needed for survey task workflows per these changes, because the viewer and task area change from horizontal to vertical at a wider screen width than previously.

With a survey task, and a screen width of less than 1120px (70rem) per these changes - the task area drops below the viewer (vertical layout) and the task area is wide enough that the PrimaryButtons (Back, Done, etc.) hit their max-width. The container div aligns the buttons at the start without the justify: center.

Without justify: center:

With justify: center:

goplayoutside3

LGTM!

mcbouslog added 4 commits September 12, 2024 12:27

Add usesSurveyTask view to Workflow store

4ad3c01

Pass usesSurveyTask to CurrentLayout

0b5e560

Refactor CenteredLayout with usesSurveyTask layout

9766cb7

Refactor MaxWidth per usesSurveyTask

e7cf35e

Refactor NoMaxWidth per usesSurveyTask

f2b240a

mcbouslog marked this pull request as ready for review September 13, 2024 19:24

mcbouslog changed the title ~~[DRAFT] Refactor classifier layout per survey task~~ Refactor classifier layout per survey task Sep 13, 2024

mcbouslog requested review from a team and seanmiller26 September 13, 2024 19:24

goplayoutside3 self-assigned this Sep 16, 2024

goplayoutside3 reviewed Sep 16, 2024

View reviewed changes

Rename usesSurveyTask to hasSurveyTask

15bdccb

Center align Choice Carousel

6ebb52b

Merge branch 'master' into lib-classifier_layout-with-survey-task

3cf009b

goplayoutside3 reviewed Sep 24, 2024

View reviewed changes

eatyourgreens reviewed Sep 24, 2024

View reviewed changes

goplayoutside3 mentioned this pull request Sep 25, 2024

Styling adjustments for Tabs and AuthModal #6339

Merged

12 tasks

Remove NoMaxWidth margin auto

0eeffc9

goplayoutside3 reviewed Sep 25, 2024

View reviewed changes

goplayoutside3 approved these changes Sep 25, 2024

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Sep 25, 2024

mcbouslog added 2 commits September 25, 2024 15:11

Merge branch 'master' into lib-classifier_layout-with-survey-task

6e1de95

Merge branch 'master' into lib-classifier_layout-with-survey-task

86e06fc

mcbouslog merged commit 6b8f322 into master Sep 26, 2024
7 checks passed

mcbouslog deleted the lib-classifier_layout-with-survey-task branch September 26, 2024 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor classifier layout per survey task #6297

Refactor classifier layout per survey task #6297

mcbouslog commented Sep 12, 2024 •

edited

Loading

coveralls commented Sep 12, 2024 •

edited

Loading

goplayoutside3 left a comment

seanmiller26 commented Sep 16, 2024

mcbouslog commented Sep 17, 2024

mcbouslog commented Sep 17, 2024

mcbouslog commented Sep 17, 2024 •

edited

Loading

seanmiller26 commented Sep 18, 2024

mcbouslog commented Sep 18, 2024

seanmiller26 commented Sep 18, 2024 •

edited

Loading

seanmiller26 commented Sep 18, 2024

mcbouslog commented Sep 23, 2024 •

edited

Loading

seanmiller26 commented Sep 23, 2024

goplayoutside3 left a comment

eatyourgreens Sep 24, 2024

goplayoutside3 Sep 24, 2024 •

edited

Loading

goplayoutside3 Sep 24, 2024

eatyourgreens Sep 24, 2024

eatyourgreens Sep 25, 2024

mcbouslog Sep 25, 2024

eatyourgreens Sep 25, 2024

mcbouslog commented Sep 25, 2024

goplayoutside3 Sep 25, 2024

mcbouslog Sep 25, 2024

goplayoutside3 left a comment

Refactor classifier layout per survey task #6297

Refactor classifier layout per survey task #6297

Conversation

mcbouslog commented Sep 12, 2024 • edited Loading

Package

Linked Issue and/or Talk Post

Describe your changes

How to Review

Checklist

General

General UX

Refactoring

coveralls commented Sep 12, 2024 • edited Loading

goplayoutside3 left a comment

Choose a reason for hiding this comment

seanmiller26 commented Sep 16, 2024

mcbouslog commented Sep 17, 2024

mcbouslog commented Sep 17, 2024

mcbouslog commented Sep 17, 2024 • edited Loading

seanmiller26 commented Sep 18, 2024

mcbouslog commented Sep 18, 2024

seanmiller26 commented Sep 18, 2024 • edited Loading

seanmiller26 commented Sep 18, 2024

mcbouslog commented Sep 23, 2024 • edited Loading

seanmiller26 commented Sep 23, 2024

goplayoutside3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goplayoutside3 Sep 24, 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

mcbouslog commented Sep 25, 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

mcbouslog commented Sep 12, 2024 •

edited

Loading

coveralls commented Sep 12, 2024 •

edited

Loading

mcbouslog commented Sep 17, 2024 •

edited

Loading

seanmiller26 commented Sep 18, 2024 •

edited

Loading

mcbouslog commented Sep 23, 2024 •

edited

Loading

goplayoutside3 Sep 24, 2024 •

edited

Loading