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

Style thumbnail placeholders nicely #5880

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jul 17, 2024

As expected, the only changes we need to make for thumbnail support #5875 are to the styling for the placeholder before a thumbnail is ready.

Along the way we also fix a quirk which I'd very occasionally spotted before and always looked wrong, but hadn't been a priority.

Thanks to @alexmv for help testing these changes. Chat thread: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/thumbnails/near/1892137


content: Use object-fit: scale-down to fit images, vs. contain

This matches the behavior of Zulip web.

The meaning of this value is that the image is either scaled
according to contain, or not scaled at all, whichever makes it
smaller.

In other words, we still scale down all the images we were scaling
down, but we stop scaling any images up, which tended to look a bit
silly. If the user wants to see the image scaled up, they can
always tap to see it in the lightbox.

This case is about to become more common with the advent of
thumbnailing: when we get a message where the server hasn't yet
gotten to producing a thumbnail, we'll show a placeholder image.
That placeholder is a 30x30 SVG, which looks rather odder if
scaled up to 160px high.


content: Show spinner for thumbnail placeholders

This is a port of zulip/zulip#30477. That PR isn't yet merged, but
it's been deployed on chat.zulip.org and I believe the styling is
unlikely to change. (If it does get tweaked, we can always update.)

The placeholder img elements already come with a src pointing
to this same loader-black.svg file, but on the Zulip server.
So the effects of this change are:

  • The placeholder image is local instead of remote, which may
    improve the experience a bit on a slow connection.

  • In dark theme, we use an appropriately contrasting white spinner,
    instead of a black spinner that blends in with the background.

Fixes: #5875


More specifically that latter commit is a port of this commit:
4d12bee29 thumbnail: Show the right spinner based on dark/light mode.

but that PR branch will presumably be rebased before merge.

gnprice added 2 commits July 16, 2024 22:19
This matches the behavior of Zulip web.

The meaning of this value is that the image is either scaled
according to `contain`, or not scaled at all, whichever makes it
smaller.

In other words, we still scale down all the images we were scaling
down, but we stop scaling any images up, which tended to look a bit
silly.  If the user wants to see the image scaled up, they can
always tap to see it in the lightbox.

This case is about to become more common with the advent of
thumbnailing: when we get a message where the server hasn't yet
gotten to producing a thumbnail, we'll show a placeholder image.
That placeholder is a 30x30 SVG, which looks rather odder if
scaled up to 160px high.
This is a port of zulip/zulip#30477.  That PR isn't yet merged, but
it's been deployed on chat.zulip.org and I believe the styling is
unlikely to change.  (If it does get tweaked, we can always update.)

The placeholder `img` elements already come with a `src` pointing
to this same `loader-black.svg` file, but on the Zulip server.
So the effects of this change are:

 * The placeholder image is local instead of remote, which may
   improve the experience a bit on a slow connection.

 * In dark theme, we use an appropriately contrasting white spinner,
   instead of a black spinner that blends in with the background.

Fixes: zulip#5875
@gnprice gnprice requested a review from chrisbobbe July 17, 2024 05:24
@gnprice gnprice mentioned this pull request Jul 17, 2024
@gnprice
Copy link
Member Author

gnprice commented Jul 17, 2024

Screenshots before/after of a 48x48 Zulip logo, to test the first commit:

before after
image image

Screenshots of a thumbnail placeholder:

before, dark after, dark after, light
image image image

(The "before" is from @alexmv here.)

@chrisbobbe
Copy link
Contributor

LGTM, thanks! Merging.

@chrisbobbe chrisbobbe merged commit 8ebb563 into zulip:main Jul 18, 2024
1 check passed
@gnprice gnprice deleted the pr-thumbnail branch July 18, 2024 23:27
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.

Make use of thumbnails
2 participants