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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Sep 10, 2024 •
edited
Loading

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 •
edited
Loading

Coverage Status

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

@eatyourgreens eatyourgreens mentioned this pull request Sep 10, 2024
Open
@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Sep 20, 2024 •
edited
Loading

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.)

goplayoutside3
goplayoutside3 reviewed Oct 2, 2024
View reviewed changes
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 •
edited
Loading

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.

@eatyourgreens
fix(classifier): proportional height for subject images
07da390 
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
Reviewers

@goplayoutside3 goplayoutside3 goplayoutside3 left review comments

Assignees

@goplayoutside3 goplayoutside3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subject viewer changes size during classification when 'limit subject height' is enabled
3 participants
@eatyourgreens @coveralls @goplayoutside3