hippware / rn-chat

MIT License
5 stars 0 forks source link

Photo Grid: Do not resize images uploaded on photo grid #386

Closed zavreb closed 7 years ago

zavreb commented 7 years ago

Goal: When users tap on images within the photo grid, the image uploaded is resized by the following library https://github.com/marcshilling/react-native-image-picker. According to @aksonov this library does not have the option to upload without modification. Please find and implement a library (or adjust current one) that allows users to upload images and render them as is when they are tapped on the photo grid. (Please note: Thumbnails and cover images are a different issue).

Req:

Image Uploaded (expected image load):

image

Image Rendered on TR:

image

aksonov commented 7 years ago

So what is maximal width, height for an image?

thescurry commented 7 years ago

@bengtan (cc: @zavreb) I'm not sure how helpful this will be, but the image should not exceed the max width of the screen. For example, look at any image on instagram, the width of the image never exceeds the width of the screen (though the height is variable for obvious reasons). So it's hard to say what the maximal width should be, but typically the image should consume the width of the screen and the screen should render the whole image (and not a crop of the image). Happy to expand on this with screenshots from instagram if that would help further clarify what I'm trying to convey.

One example would be the top image that Becky listed called (expected image load). What's important is to make sure we show the entire image and that we make the image width match the screen size horizontally. We can fill in the vertical space with a block color like white/black per the example Becky posted above.

bengtan commented 7 years ago

I'm not sure how helpful this will be, but the image should not exceed the max width of the screen.

It's somewhat off-topic from the original post and I think it deserves its own ticket:

Images should be displayed as fit-to-screen

When an image is displayed in detail, it should default to fit-to-screen. In other words, the entire image should be displayed on the screen. If the image proportions do not match the screen exactly, then black 'filler' regions shold be displayed on the left and right (or top and bottom) of the image.

@zavreb: Feel free to plagiarise and create a new ticket.

aksonov commented 7 years ago

I believe it is already done (fit-to-screen), right?

On 22 Feb 2017, at 08:50, Beng Tan notifications@github.com wrote:

I'm not sure how helpful this will be, but the image should not exceed the max width of the screen.

It's somewhat off-topic from the original post and I think it deserves its own ticket:

Images should be displayed as fit-to-screen

When an image is displayed in detail, it should default to fit-to-screen. In other words, the entire image should be displayed on the screen. If the image proportions do not match the screen exactly, then black 'filler' regions shold be displayed on the left and right (or top and bottom) of the image.

@zavreb https://github.com/zavreb: Feel free to plagiarise and create a new ticket.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/hippware/rn-chat/issues/386#issuecomment-281594543, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQpcboaSY2kU15uW78v0xKKzGKXVseWks5re-jFgaJpZM4L7ODT.

bengtan commented 7 years ago

Back to the original topic.

@aksonov thinks we should have resizing so as to reduce bandwidth and storage.

For example, if someone has a 10000x10000 px image, do we really want to upload it in full size? Probably not.

If someone has a 1000x1000px image, then probably we do want to upload it in full size.

So @aksonov wants to know what the limit should be. Photos under this limit can be uploaded without ~with~ resizing. Photos over this limit will be resized (ie. make smaller) before uploading.

zavreb commented 7 years ago

@thescurry wants to go with 1000 x 1000px as our starting point

This makes sense, we'll continue to improve from here:

So @aksonov wants to know what the limit should be. Photos under this limit can be uploaded without with resizing. Photos over this limit will be resized (ie. make smaller) before uploading. 👍 1

zavreb commented 7 years ago

@aksonov do you need this in a new ticket or keep it in here?

aksonov commented 7 years ago

Let’s keep it here.

On 22 Feb 2017, at 23:41, Rebecka Z notifications@github.com wrote:

@aksonov https://github.com/aksonov do you need this in a new ticket or keep it in here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hippware/rn-chat/issues/386#issuecomment-281829089, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQpcd62ueRTBQ0_76ikhCfgnZhv-gsnks5rfLmsgaJpZM4L7ODT.

bengtan commented 7 years ago

scurry wants to go with 1000 x 1000px as our starting point

1000x1000 seems a bit low.

iPhone 7 Plus screen is 1920x1080. I think it should be at least 1920x1920 so either of ...

1920x1920 or 3000x3000 [1].

Also, the server side is also downsizing large images (https://github.com/hippware/wocky/issues/517) so there is some duplication.

[1] 1920x1920 will bypass server side resizing. 3000x3000 is sufficiently larger than 1920 that resizing won't degrade image quality much.

bengtan commented 7 years ago

Maybe let's discuss this in our weekly meeting.

zavreb commented 7 years ago

@aksonov not sure why this is in "Ready for QA"; don't see the fix

This will only address new images.

Including in bug sprint.

bengtan commented 7 years ago

The changes for this ticket in 1.24.0 resulted in a new issue:

Photos have too large filesize (1.24.0) https://github.com/hippware/rn-chat/issues/433

aksonov commented 7 years ago

I still don't see comparison for 1.24.0 screenshots and original ones....

zavreb commented 7 years ago

On Monday (before the AWS outage) portrait images were not being cropped, however landscape images were.

Today both are being cropped.

mstidham commented 7 years ago

Staging Version 1.24.2 (26) iPhone 7 Plus Version 10.2.1 This image is a landscaped image taken on iPhone 7 plus. image

zavreb commented 7 years ago

Portrait Image with Staging Version 1.24.2 (26) i7 plus

image

bengtan commented 7 years ago

@zavreb:

Regarding cropping of images, I've got @bernardd to have a look so please give us a day or so to get back to you.

Meanwhile, I think 'cropping of images' can be spun out into a separate ticket.

bengtan commented 7 years ago

Okay, Bernard has come back with some investigation. The images, as stored on the server side / S3, are not cropped.

So if something incorrect is happening, it's after the client has downloaded the images from S3.

Looking more closely at the screenshots, to me, it appears as if the client just isn't 'fit-to-screen'-ing correctly, rather than some weird intentional cropping happening.

In other words, the images don't fit nicely onto the screen and there are some parts which are outside of the viewing area and hence, can't be seen.

zavreb commented 7 years ago

Close per @aksonov's suggestion, will review after meeting.

mstidham commented 7 years ago

Verified on staging

zavreb commented 7 years ago

Verified on Prod. Closing ticket.