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

Allow cropping an avatar before setting it #32565

Merged
merged 32 commits into from
Nov 28, 2024
Merged

Conversation

kerwin612
Copy link
Member

@kerwin612 kerwin612 commented Nov 19, 2024

In response to the issues raised in #31990, and related discussions in #31991;
I tend to agree with @silverwind's viewpoint (using object-fit: cover; is equivalent to actively cropping the image for the user, which cannot guarantee the crop is what the user wants).
Therefore, I believe this issue should be addressed in two parts:
①, Add a border to the user's avatar, similar to how GitHub does it; this way, unconventionally proportion images configured through the API won't look as ugly as mentioned in the issue.
②, Provide a cropping tool on the avatar editing page, allowing users to select the cropping area themselves. This way, users can decide the displayed area of the image, rather than us deciding for them.

This PR is the solution for part ②; I will propose the solution for part ① in another PR.
after

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 19, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 19, 2024
@kerwin612 kerwin612 marked this pull request as draft November 19, 2024 15:30
@kerwin612 kerwin612 marked this pull request as ready for review November 19, 2024 15:48
@kerwin612 kerwin612 changed the title Fixed #31990 Add cropping support for avatar editing Nov 20, 2024
@lunny lunny added this to the 1.23.0 milestone Nov 20, 2024
@wxiaoguang
Copy link
Contributor

  1. Do not use single word for names (see the frontend guideline)
  2. Do not pollute global CSS classes like "hidden"
  3. Do users all agree to only use "jpeg" file as avatar? IIRC there were requests to support animated png and webp before. I do not mean which decision is right, but if the behavior is changed, there should be a explanation for this breaking change.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2024
@kerwin612
Copy link
Member Author

  1. Do not use single word for names (see the frontend guideline)
  2. Do not pollute global CSS classes like "hidden"
  3. Do users all agree to only use "jpeg" file as avatar? IIRC there were requests to support animated png and webp before. I do not mean which decision is right, but if the behavior is changed, there should be a explanation for this breaking change.

Thank you for the suggestion. The code has been changed according to the specifications. Additionally, I have added the necessary prompt to inform users that cropped images will be saved in JPEG format uniformly.

@silverwind
Copy link
Member

silverwind commented Nov 20, 2024

  1. If we have to decide on a single output format, it should be PNG imho, not JPEG. Lossy formats suck.
  2. When the user does not crop, pass through the image unmodified.

Gitea already supports formats like animated PNG and WEBP for avatars, so with the passthrough, they should continue to work.

@kerwin612
Copy link
Member Author

code optimization

Thank you. Both requirements have been implemented.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 27, 2024

Made some changes in 5f893c3 , the code is also simplified:

  1. remove the "preview" image, it doesn't really help, the cropper already shows the result clearly, then the mobile display is also resolved.
  2. use field tw-pl-4 instead of inline field to make the form field content align
  3. most cropper styles are not needed, only need to limit the size
  4. make initCompCropper more general, decouple it from user avatar page
  5. make initCompCropper type-script error-free
  6. remove the unnecessary _cropper property, the JS function already have a good variable scope
  7. use viewMode: 2, to replace viewMode: 1 to avoid users zoom too much
  8. use correct filename (replace ext) and last modified time

@kerwin612 kerwin612 requested a review from silverwind November 27, 2024 10:57
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 27, 2024
@delvh delvh changed the title Add cropping support for avatar editing Allow cropping an avatar before setting it Nov 27, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 27, 2024
@GiteaBot

This comment was marked as outdated.

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 27, 2024
options/locale/locale_en-US.ini Outdated Show resolved Hide resolved
web_src/js/features/user-settings.ts Show resolved Hide resolved
@kerwin612 kerwin612 requested a review from delvh November 28, 2024 00:09
@kerwin612 kerwin612 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 28, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) November 28, 2024 01:54
@wxiaoguang wxiaoguang merged commit 68d9f36 into go-gitea:main Nov 28, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 28, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 28, 2024
* giteaofficial/main:
  Allow cropping an avatar before setting it (go-gitea#32565)
  Add webpack EnvironmentPlugin (go-gitea#32661)
  Move team related functions to service layer (go-gitea#32537)
  Make frontend unit test code could know it is in testing (go-gitea#32656)
  Add priority to protected branch (go-gitea#32286)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/frontend modifies/templates This PR modifies the template files modifies/translation size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants