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

Refactor classifier layout per survey task #6297

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mcbouslog
Copy link
Contributor

@mcbouslog mcbouslog commented Sep 12, 2024

Package

  • lib-classifier

Linked Issue and/or Talk Post

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:

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

Refactoring

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

@coveralls
Copy link

coveralls commented Sep 12, 2024

Coverage Status

coverage: 78.689% (-0.07%) from 78.755%
when pulling 3cf009b on lib-classifier_layout-with-survey-task
into a4fba3d on master.

@mcbouslog mcbouslog marked this pull request as ready for review September 13, 2024 19:24
@mcbouslog mcbouslog changed the title [DRAFT] Refactor classifier layout per survey task Refactor classifier layout per survey task Sep 13, 2024
@mcbouslog mcbouslog requested review from a team and seanmiller26 September 13, 2024 19:24
@goplayoutside3 goplayoutside3 self-assigned this Sep 16, 2024
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

A couple of things before approval:

  1. 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.
  2. 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:
Screenshot 2024-09-16 at 11 23 08 AM Screenshot 2024-09-16 at 11 22 46 AM

@seanmiller26
Copy link
Contributor

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

@mcbouslog
Copy link
Contributor Author

A couple of things before approval:

  1. 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
Copy link
Contributor Author

  1. 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
Copy link
Contributor Author

mcbouslog commented Sep 17, 2024

@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
Copy link
Contributor

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

@mcbouslog
Copy link
Contributor Author

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

Yes!

@seanmiller26
Copy link
Contributor

seanmiller26 commented Sep 18, 2024

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
Copy link
Contributor

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

Maybe this is better addressed in the choices PR.

Screenshot 2024-09-18 at 1 36 56 PM Screenshot 2024-09-18 at 1 37 05 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
4 participants