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

Make the classifier's subject viewer sticky #6103

Merged
merged 4 commits into from
May 29, 2024

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented May 23, 2024

Package

lib-classifier

Linked Issue and/or Talk Post

Toward: #6063

Describe your changes

  • This change is intended to improve volunteer's classifying experience when a workflow has a "tall" task area and landscape oriented subject viewer. For example, when a volunteer clicks "Done" on a survey task, we want them to keep the subject in view rather than forced to scroll up and down the page to complete each classification.
  • Added sticky css styling to the ViewerGrid component for NoMaxWidth and MaxWidth layout. MaxWidth layout is the default layout and NoMaxWidth layout is specific to transcription projects.
  • The CenteredLayout's task area wasn't being sticky (e.g Daily Minor Planet) especially for separate frames view, so I fixed that. CenteredLayout only applies when a project team configures their workflow to "limit subject height".

How to Review

A few types of projects to review are below. The full list of FEM projects is here.

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

New Feature

  • The PR creator has listed user actions to use when testing the new feature
  • Unit tests are included for the new feature
  • A storybook story has been created or updated
    • Example of SlideTutorial component with docgen comments, and its story

@goplayoutside3 goplayoutside3 added the enhancement New feature or request label May 23, 2024
@@ -4,7 +4,6 @@ export default function getLayout(layout) {
if (layouts[layout]) {
return layouts[layout]
} else {
console.warn(`Couldn't find a layout for '${layout}', falling back to default`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opinionated statement, but this warning was just cluttering the console during unit tests.

@coveralls
Copy link

coveralls commented May 23, 2024

Coverage Status

coverage: 80.425% (+0.02%) from 80.401%
when pulling 328f69c on sticky-subject-viewer
into de180dc on master.

@goplayoutside3 goplayoutside3 requested a review from a team May 23, 2024 17:17
@seanmiller26
Copy link
Contributor

This is a BIG step in the right direction. Super useful to keep the subject in view at all times. I tried every preview link and they all work great. OK by me to move forward with implementing this.

@seanmiller26
Copy link
Contributor

seanmiller26 commented May 23, 2024

I have 2 small questions about page positioning and scrolling which I chatted about briefly with Delilah.

  1. When submitting a classification, you will not be brought back up to the top of the page and the new subject sometimes loads out of view.
Screen.Recording.2024-05-23.at.3.17.44.PM.mov
  1. When you are in a survey selection modal and you choose 'Not this,' you are brought back to the very top of the page. It can be a pretty jarring interaction.
Screen.Recording.2024-05-23.at.3.25.14.PM.mov

I'd love to unify the behavior here. Either they both bring you back to the top of the page (preferably just above the subject, no need to see the header and project header), or you will retain your page position for both.

@goplayoutside3
Copy link
Contributor Author

  1. I think the video example here illustrates behavior that is purposely in the volunteer's control. If a volunteer clicks Done while the Done button is in the vertical middle of their screen, then it's not a problem for the Done button to remain in the vertical middle of their screen.

  2. However, this video is something I'll look into because it's also jarring on production, so seems to be caused by something else regardless of a sticky subject viewer. Will report back!

@goplayoutside3
Copy link
Contributor Author

goplayoutside3 commented May 25, 2024

The jarring scroll-to-top behavior with the survey task specifically is because the Chooser component (all the animal options), and the Choice component (form that collects final answer) are drastically different heights. Only one renders at a time, and usually the Choice component is much taller than the Chooser because it includes example images, paragraphs, select buttons, etc. If a volunteer selects an animal to identify, they see the Choice component, scroll down so the "Identify" button is in the middle of their screen vertically, click "Identify", but then the Choice component is replaced with the shorter Chooser component. This throws off window positioning regardless of whether the subject area is sticky. One way to solve this is to make the survey tasks's Choice component scrollable in an isolated manner, rather than scrolling the entire page.

However, there's a similar problem for projects that have a super tall task area like: md68135/notes-from-nature-labs which has a combo task of 4 single answer questions. A volunteer must scroll down the page to answer all four questions and click Next or Done which causes the window to scroll almost all the way to the top.

To summarize, the cause of the window jumping back up to the top when a volunteer clicks buttons in the task area is because it changes height while the subject image height remains the same, and the browser takes a best guess about position. In a perfect world, all components of the classifier would be designed to fit inside a volunteer's screen height for an immersive experience. Making the subject viewer sticky is a step toward that and I would still like this PR reviewed as is.

I'm hesitant to manually manipulate users' scroll position, but something we could explore is a function triggered by Done, Next, Back, and the survey task Identify buttons that says "if the top edge of the classifier is above a user's viewport, scroll their screen so the top edge of the classifier is at the top edge of their viewport". In a separate PR though.

@mcbouslog mcbouslog self-assigned this May 28, 2024
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

This looks good! Tested on noted projects without issue.

@github-actions github-actions bot added the approved This PR is approved for merging label May 29, 2024
@mcbouslog
Copy link
Contributor

mcbouslog commented May 29, 2024

Are we concerned with mobile browser behavior?
The task area covers the subject on mobile as a user scrolls.
I think it's ok, but double-checking.

Uploading TESSmobile.mp4…

@goplayoutside3
Copy link
Contributor Author

Hmmm I think I see what you mean on mobile. The video in your comment didn't upload, but I'll remove all sticky behavior for mobile-sized viewports before merging this PR!

@goplayoutside3 goplayoutside3 merged commit 952fdb6 into master May 29, 2024
8 checks passed
@goplayoutside3 goplayoutside3 deleted the sticky-subject-viewer branch May 29, 2024 18:58
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.

None yet

4 participants