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

Constrain image heights in feeds and threads #5129

Merged
merged 16 commits into from
Sep 5, 2024
Merged

Constrain image heights in feeds and threads #5129

merged 16 commits into from
Sep 5, 2024

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Sep 4, 2024

Constrains the height of images in feeds and threads.

  • anything shorter than 1:1 appears in-full
  • anything taller than 1:1 appears "contained" by it's parent height (a square, so proportional to column width)
  • anything taller than 3:4 crops within a min-sized container (matches common photo size)
  • in highlighted thread view, images are not cropped
    • unless very tall, they're cropped to 1:4, but still appear full-width
  • QP with media crops embed image to 1:1 to save space

To do

  • check on wiiiide image case
  • add image bg for multi-image posts

Cases

Too tall, crops to min-sized container
CleanShot 2024-09-03 at 20 48 21@2x

Same in quotes
CleanShot 2024-09-03 at 20 48 34@2x

Quotes with media appear slightly differently
CleanShot 2024-09-04 at 11 12 29@2x

Image shorter than 1:1 appears as it's normal crop
CleanShot 2024-09-03 at 20 49 03@2x

Very wide images display in full (TBD if we want this)
CleanShot 2024-09-03 at 20 56 01@2x

All images now have a background, so that while loading there's something to take up space
CleanShot 2024-09-03 at 20 49 31@2x
CleanShot 2024-09-03 at 21 07 04@2x

Works in DMs too
CleanShot 2024-09-04 at 10 38 12@2x

Copy link

render bot commented Sep 4, 2024

Copy link

github-actions bot commented Sep 4, 2024

Old size New size Diff
7.09 MB 7.1 MB 4.45 KB (0.06%)

<PostEmbeds
embed={post.embed}
moderation={moderation}
contextView="thread-highlighted"
Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving this open ended in case we want to add additional cases

onPressIn?: () => void
style?: StyleProp<ViewStyle>
children?: React.ReactNode
export function useImageAspectRatio({
Copy link
Member Author

Choose a reason for hiding this comment

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

Just abstracted this to a hook

@DavidBuchanan314
Copy link
Contributor

A pet peeve of mine is when cropped images offer no visual indication that they've been cropped. Depending on the actual image content, it can be non-obvious that there's missing information that must be clicked to be seen.

Here's a mockup of an idea I had, setting a 4px coloured border on the top and bottom:

image

(the image in the mockup isn't actually cropped, just imagine it's taller heh)

@mary-ext
Copy link
Contributor

mary-ext commented Sep 4, 2024

Is it not doable to scale it down instead? I suppose in React Native this would mean having to do a layout measurement but it's also something that only have to be done once (shared to all post component instances within a context)

I'd expect it to crop at more extreme aspect ratios, like where it isn't possible to display in its entirety with the minimum height/width constraint of ~32/128px

@estrattonbailey
Copy link
Member Author

@mary-ext you thinking like this? Totally doable to allow smaller sizes, but starts to look kinda awkward aesthetically in a feed of posts. The 3:4 (portrait) minimum was chosen specifically to fit the most common photo size, though I think some Androids ship with 16:9 (or 9:16 portrait) by default.

CleanShot 2024-09-04 at 09 28 42@2x

@estrattonbailey
Copy link
Member Author

@DavidBuchanan314 that's a fun idea, will play with it!

@estrattonbailey estrattonbailey marked this pull request as ready for review September 4, 2024 16:59
@surfdude29
Copy link
Contributor

surfdude29 commented Sep 4, 2024

I've just been trying this out on the preview branch and it works really well, great job! :)

I especially like the little crop icon shown on solo cropped images.

I did notice one strange glitch: this quote post in Following had a strangely rendered link card in the quoted post:

IMG_9634

It also shows up like this when scrolling through posts on the user's profile. However, the link card shows up fine when viewing the post by itself.

@surfdude29
Copy link
Contributor

Also, the interaction with the content hider in this quote post is not great:

IMG_9642

@estrattonbailey
Copy link
Member Author

@surfdude29 ah thanks for the QA! Actually working on this issue right now 👍

@surfdude29
Copy link
Contributor

@estrattonbailey Oh awesome 👌

@Tanza3D
Copy link
Contributor

Tanza3D commented Sep 5, 2024

https://bsky.app/profile/tanza.me/post/3l3fn5yc5fw2n
seems these very specific image resolutions make it explode
image

image
doesn't happen when only two are attached

@Tanza3D
Copy link
Contributor

Tanza3D commented Sep 5, 2024

also, how's the thoughts on centering it when it's too tall and having a backdrop? kinda like how tall images display on anthera

image

@estrattonbailey
Copy link
Member Author

@Tanza3D thanks for the QA help! Ah yes, I see that in Firefox. Will have a look!

@DavidBuchanan314
Copy link
Contributor

I'm a fan of the crop icon, definitely solves my "invisible cropping" gripe

@estrattonbailey
Copy link
Member Author

@DavidBuchanan314 thanks for the suggestion! We're gonna leave it off multi-image posts for now since it gets kinda aesthetically busy, but it'll appear on single-image posts if they're cropped 👍

@DavidBuchanan314
Copy link
Contributor

Makes sense, when I see a multi-image post my default assumption is that they're cropped

@haileyok
Copy link
Contributor

haileyok commented Sep 5, 2024

@haileyok
Copy link
Contributor

haileyok commented Sep 5, 2024

@estrattonbailey
Copy link
Member Author

Yep yep should be fixed in latest comment, just hasn't deployed on Render yet 👍

Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

All initial indicators look good. Amazing work, and so happy we got this done.

Lets just let it bake for a couple days and make sure we dont notice any jank.

@estrattonbailey estrattonbailey merged commit 2265fed into main Sep 5, 2024
6 checks passed
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.

6 participants