Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Restrict size of image and video thumbnails #5682

Closed
wants to merge 6 commits into from

Conversation

williamkray
Copy link

@williamkray williamkray commented Feb 24, 2021

While my previous PR included changes to the URL preview widget that didn't go over well, community response seemed overwhelmingly positive for the changes that made the image and video thumbnails smaller. This PR is just those two changes.

Original community discussion about the changes, for reference

  • "However the smaller image previews are a godsend. Surprisingly significant improvement to my user experience."
  • "While I agree with the feedback regarding URL previews, it also limits the size of image files sent in the timeline, which I like because it makes the room timeline much more readable."
  • "I agree that the smaller image previews are amazing."

However, it seems like this might still be up for conversation from the design team due to Matthew's response:

Meanwhile it was a very deliberate choice when we designed Riot to go for large immersive image thumbnails, and suddenly unilaterally switching to tiny ones feels opinionated at best.

I suspect the right solution here is going to be to make the thumbnail size adjustable... and not to artifically link URL previews and image thumbnails together, given they are completely different things.

Before:
Screenshot_20210228110740

After:
Screenshot_20210228110800

Signed-off-by William Kray [email protected]

@williamkray
Copy link
Author

i'll just keep merging in develop every once in a while until those tests pass.

@jryans
Copy link
Collaborator

jryans commented Feb 27, 2021

@williamkray Still has some test failures. Could you also add some screenshots as well?

@williamkray
Copy link
Author

i keep merging develop in, but it keeps failing those tests... which i haven't touched. this PR is only for a couple CSS changes. i'll keep trying but i have no idea what they're failing on.

screenshot comparison added.

@SimonBrandner
Copy link
Contributor

Screenshot_20210228_201652

@williamkray, as far as I can tell the CI is cloning your develop branch from matrix-js-sdk. Try merging the upstream develop of matrix-js-sdk and pushing it.

@williamkray
Copy link
Author

thanks @SimonBrandner , i didn't see that nor did i realize it would be tied to my version of the js-sdk which i haven't really touched. i've refreshed that, hopefully that fixes the testing issues next time i merge in react-sdk upstream development.

@williamkray
Copy link
Author

i just noticed Anoa's comment in my screenshot fits in nicely with this pull request, haha! that was clearly posted when the develop.element.io had the previous PR added in.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

The code looks fine, though this has conflicting opinions from design/product/management on whether it should be a thing or not.

@turt2live turt2live requested a review from a team March 22, 2021 02:46
@SimonBrandner
Copy link
Contributor

Fixes #1520

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

I'm a big fan of this change, however I think that any change to image/video sizing should strive to at least keep all different shapes of images "readable". An issue with just setting a smaller max height is that while it is quite a positive change for landscape images, portrait images become squashed and hard to glean information from.

Thus I think a better solution would be to set a max area for images, and then adapt their size based on that according to their aspect ratio, in order to maintain a consistent information density. It would be nice if such an approach could be used for videos as well, however I don't know if you're able to get the aspect ratio of a video ahead of time.

@williamkray
Copy link
Author

i definitely understand that perspective. i'm going to defend my approach here with three main points:

  • i would argue that it's less important to need to retain readability of images as thumbnails because viewing the full-sized image is natural instinct when you cannot glean the information from the thumbnail. slack even goes so far as to crop image thumbnails to a set aspect ratio, potentially dropping readability from perspective. the whole idea of the thumbnail is to give an approximation of an image without having to meet readability or resolution requirements because it is only a "preview". the full image is always accessible for that type of review.
  • secondarily, using max area and potentially still having excessively tall image thumbnails for the sake of readability still would come back to a disruptive conversational experience within the room... with limited height thumbnails, it's easier to see the context of the conversation, while clicking the image to see the full content, and then going back to the conversation by exiting the image view. long images that don't have a max height constraint would push more or potentially all previous conversation out of view, reducing context and requiring the reader to scroll up to understand why this incredibly long image has been posted.
  • but the third and final reason i would push back on this idea is because... i have no idea how to achieve it 😬 so i would need to hand this PR off to someone with the development capabilities to tackle that approach.

@turt2live
Copy link
Member

Thank you for the contribution! We've opted to go for a version of this with a setting given the tension between small & large thumbnails: #7017

@turt2live turt2live closed this Oct 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants