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

fix(classifier): proportional height for subject images #6276

Closed

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Sep 10, 2024

When 'limit subject height' is enabled, style subject images with a proportional height of 90% of the viewport height, up to a maximum size of the original image height.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

Linked Issue and/or Talk Post

How to Review

Subject height should now be fixed during all workflow tasks, but never larger than the viewport.

https://local.zooniverse.org:3000/projects/chloezycheng/science-scribbler-synapse-safari/classify/workflow/26732

Screen.Recording.2024-09-10.at.13.54.07.mov

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

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

@coveralls
Copy link

coveralls commented Sep 10, 2024

Coverage Status

coverage: 78.546% (+0.03%) from 78.512%
when pulling 07da390 on eatyourgreens:fix-subject-height
into fef769f on zooniverse:master.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Sep 20, 2024

A fixed height subject breaks the layout on small screens, so this won't work on a phone.

Having said that, the current max-height styling doesn't work on a phone either (#6061.)

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.

Unfortunately changing from max-height to height results in a bug where the viewer no longer adjusts to viewports < 1000px. The subject image must adjust as the viewport width adjusts. See https://local.zooniverse.org:3000/projects/fr/fulsdavid/the-daily-minor-planet/classify/workflow/22354?env=production&demo=true as an example where subject images are always square images.

🚨 Here's a screenshot of my phone's Safari browser with this branch run locally. The image is squished:
Daily Minor Planet on a mobile device

@goplayoutside3
Copy link
Contributor

@eatyourgreens I realize I'm repeating what you commented about phones earlier, but how do you want to proceed with this PR? It introduces a bug. For instance, I can look at the production version of Daily Minor Planet on my phone and the subject image is scaled correctly. That's changed in this PR.

@goplayoutside3 goplayoutside3 self-assigned this Oct 2, 2024
@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 2, 2024

I'm not sure. CSS height must always be specified alongside max-height, otherwise the browser calculates the used height, resulting in subjects that change height as you move through a workflow (#6275.) I'd suggest fixing the invalid CSS styles, but I'm not sure what the height should be set to on a phone.

@eatyourgreens
Copy link
Contributor Author

I'd suggest using maxHeight on smaller screens but that introduces #6061, which makes Squirrel Mapper much harder on a phone.

When 'limit subject height' is enabled, style subject images with a proportional height of 90% of the viewport height, up to a maximum size of the original image height.
@eatyourgreens
Copy link
Contributor Author

I played around with this a bit more tonight but couldn't get it to work on tablet-sized screens.

@eatyourgreens eatyourgreens deleted the fix-subject-height branch October 3, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subject viewer changes size during classification when 'limit subject height' is enabled
3 participants