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

Add cropping support for avatar editing #32565

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

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.

@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 20, 2024
@silverwind
Copy link
Member

silverwind commented Nov 20, 2024

Can we put the cropping content into a fomantic modal instead of dumping it into the page body like that?

Edit: Or maybe it's okay, given that this form requires to hit the submit button below anyways.

@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 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. 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.

5 participants