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

Use object-cover for author images unless AR is really high or low #3492

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Oct 9, 2024

While working on lazy loading the Author's page, I felt that we're using object-contain on author images too much - while this works nicely for covers, I feel (subjecively) that the author images look unpolished when we use object-contain. So I modified the current decision algorithm for using object-contain to only use it when the AR is very high or low.

Here's my author page before:
Screenshot 2024-10-09 150942

And after:
Screenshot 2024-10-09 151103

Some of the more significant changed are marked on the After screenshot.

I guess it's really subjective, but I find that I like the After much more, even though in some cases (e.g. John Scalzi), it results in a slightly cut image - but I think in those cases the original looks quite bad as well.

I guess I should also work on allowing users to upload their own author image...

@mikiher mikiher marked this pull request as ready for review October 9, 2024 12:27
@advplyr
Copy link
Owner

advplyr commented Oct 9, 2024

I agree it looks better for my authors also.
We can probably remove that functionality altogether and use object-cover. An image with an aspect ratio < 0.5 or > 2 will probably display worse when it uses object-contain.
Maybe after the next release if users like using object-cover

@advplyr advplyr merged commit a6da324 into advplyr:master Oct 9, 2024
5 checks passed
@mikiher mikiher deleted the author-image-ar branch November 18, 2024 09:36
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.

2 participants