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

Fix the complete display of avatar #31991

Closed
wants to merge 1 commit into from
Closed

Conversation

seepine
Copy link

@seepine seepine commented Sep 6, 2024

Fix #31990

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 6, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 6, 2024
@techknowlogick techknowlogick added type/bug topic/ui Change the appearance of the Gitea UI backport/v1.22 This PR should be backported to Gitea 1.22 labels Sep 6, 2024
@lunny
Copy link
Member

lunny commented Sep 6, 2024

Can you paste the screenshot before and after?

@seepine
Copy link
Author

seepine commented Sep 6, 2024

Before

image

After

image

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Non-quadratic images might now look strange, but I think that's okay.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 6, 2024
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

I think I'm against it. cover will crop the avatar to the smallest dimension. I see more value in showing the full avatar. For example https://en.wikipedia.org/wiki/Longcat#/media/File:Longcat_is_loooooooooong.jpg:

contain:

image

cover (head cut off):

image

Ideally we should provide a cropping UI for avatar uploads, until then users need to pre-crop their images:

image

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 6, 2024
@seepine
Copy link
Author

seepine commented Sep 7, 2024

Of course the cropped like GitHub would be better.
But cover has achieved the closest effect, solving the current problem.
Unfortunately i wirte vue only and unable to complete this function, i will close this pr if the cover is unnecessary.

@silverwind
Copy link
Member

silverwind commented Sep 10, 2024

I'd keep it as-is with contain as that ensures no cut-off content which is arguably better then the auto-cropping of contain which may crop wrongly. We should integrate a JS solution to let the user crop the image before upload. Maybe I will contribute that later. Sorry to close your first PR, maybe you find other areas to contribute.

@silverwind silverwind closed this Sep 10, 2024
@lunny lunny removed the backport/v1.22 This PR should be backported to Gitea 1.22 label Sep 11, 2024
@seepine
Copy link
Author

seepine commented Sep 13, 2024

I'd keep it as-is with contain as that ensures no cut-off content which is arguably better then the auto-cropping of contain which may crop wrongly. We should integrate a JS solution to let the user crop the image before upload. Maybe I will contribute that later. Sorry to close your first PR, maybe you find other areas to contribute.

My English is not good; if my expression is unclear, you can contact me in the QQ group chat 328432459.

  1. I believe that the cropping feature and setting the avatar image as a cover do not conflict. The cover is designed to ensure that the image is displayed as expected. Even if you add a cropping feature, if users directly modify the image using the API, it will still result in the display issues I mentioned.
  2. I am skeptical about the example you provided; it's clear that displaying the image in its entirety would be better than having an empty area in the avatar. The 'contain' option seems to be acting like a bug.
  1. In version 1.22.0, uploading an avatar directly results in it being cropped to a cover effect. Setting the image display to 'cover' is actually a way to maintain compatibility with older versions and keep the display effect consistent. You can't wait for the feature to go live and then notify each user with an incorrectly set avatar to update their profile picture one by one.
  2. Discord、bilibili、juejin, they also use cover.

Img

Discord

image

bilibili

image

juejin

image

@seepine
Copy link
Author

seepine commented Sep 13, 2024

Not cover, it like a bug.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to display complete avatar
6 participants