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(lib-react-components): thumbnail image sizing #6360

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

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Oct 4, 2024

Remove the Grommet fill prop from thumbnail images, which should fix odd sizing problems in markdown images.

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

Package

  • lib-react-components

Linked Issue and/or Talk Post

How to Review

yarn bootstrap:es6 to build the component libraries and Next.js apps, then run the project app locally with yarn start.

Shaun's test project has some images in markdown tables:
https://local.zooniverse.org:3000/projects/darkeshard/test-project-2022/about/results

Sahaun's test page, showing that the tiny thumbnail image is fixed.

WildWatch Burrowing Owl has a bunch of thumbnail image components in the tutorial, field guide and survey task. None of them should have changed on this branch.
https://local.zooniverse.org:3000/projects/sandiegozooglobal/wildwatch-burrowing-owl/classify/workflow/26938

The new home page might be using the ThumbnailImage component to render recent subjects, see #6333. You can run app-root locally to verify that those haven't changed size.

The styling changes here shouldn't have changed the thumbnail images in the storybook, or in the Markdownz stories either.

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 Oct 4, 2024

Coverage Status

coverage: 77.73% (+0.001%) from 77.729%
when pulling 400c22a on eatyourgreens:thumbnail-image-sizing
into 856b3bc on zooniverse:master.

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 this PR does unexpectedly change the size of images in markdown. Images are no longer constrained to their container without fill (height: 100%, width: 100%).

Planet Hunters TESS has a bunch of images in its field guide, and when run on this branch, they push the field guide width way off the viewport. See https://local.zooniverse.org:3000/projects/nora-dot-eisner/planet-hunters-tess/classify/workflow/11235?env=production

The unexpected image behavior is also visible in lib-react-component's Storybook. Here's a screenshot of production:
Image with 100% width

And a screenshot on this branch:
Unconstrained image

@eatyourgreens
Copy link
Contributor Author

Oh, that’s annoying, but it explains why the rule is there. I was able to reproduce the original bug in PFE, by adding width: 100%; to its markdown image styles.

Layout tables in HTML are a bad practice, so maybe the solution is to discourage people from using them.

Remove the Grommet `fill` prop from thumbnail images, which should fix odd sizing problems in markdown images.
@eatyourgreens
Copy link
Contributor Author

The unexpected image behavior is also visible in lib-react-component's Storybook.

I missed this because I've been looking at the storybook on a widescreen monitor. 🤦 I can reproduce it if I shrink my browser window too.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 17, 2024

@goplayoutside3 I can fix the storybook by adding max-width: 100%; to the image styles, which I think matches the PFE image styling. That might be the best fix, for backwards compatability.

Markdown image styles in the dev console, from PFE. Images are styled with max-width: 100%.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 17, 2024

I'll push a commit with that styling change, but feel free to close this if it creates more problems than it solves.

I've checked it locally with PH-TESS and Wildwatch Burrowing Owl, and it seems OK.

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.

Markdown images (ThumbnailImage) have inconsistent sizing behaviour
3 participants