Skip to content

Upgrade cropperjs to v2, add avatar id and link to it #33827

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 9, 2025

Update cropperjs to v2.

Potential upstream issue: fengyuanchen/cropperjs#1233 No longer needed, we now re-create the element every time instead of updating.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 9, 2025
@silverwind silverwind marked this pull request as draft March 9, 2025 04:20
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.

LGTM once the TS issue is fixed

@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 Mar 10, 2025
@silverwind
Copy link
Member Author

silverwind commented Mar 11, 2025

There's also the initial area selection not working for some reason. It previously selects 50% of the area space, I think I would like to make that 100%.

@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Mar 14, 2025
@silverwind
Copy link
Member Author

Still not fully working, but I pushed one related improvement: Clicking on the current user's avatar now adds the #avatar url fragment which scrolls the user settings page to that section.

@silverwind
Copy link
Member Author

All working now. The only noticeable difference to before is that the checkered background is removed now, because I find it doesn't look good on dark theme. Here is how it looks without and with selection:

Screenshot 2025-03-14 at 10 24 38 Screenshot 2025-03-14 at 10 24 52

@silverwind silverwind marked this pull request as ready for review March 14, 2025 09:26
@silverwind silverwind changed the title Upgrade cropperjs to v2 Upgrade cropperjs to v2, add avatar id and link to it Mar 14, 2025
@silverwind silverwind requested a review from wxiaoguang March 14, 2025 10:36
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

ping original author @kerwin612

@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 Mar 17, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 17, 2025

The selection area could be dragged out of the image, then the generated image will have a lot of space.

(It is impossible in v1 version)

image

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 17, 2025

It overflows on mobile view, v1 has a scrollbar .......

image

</cropper-selection>
</cropper-canvas>
`);
cropperCanvas.querySelector<CropperSelection>('cropper-selection').addEventListener('change', debounce(async (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This event is not right. For example: "zoom" won't trigger "change" event and users will get wrong image.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally I would like to disable zooming, but found no way to do so.

@wxiaoguang wxiaoguang marked this pull request as draft March 17, 2025 02:15
@silverwind
Copy link
Member Author

The selection area could be dragged out of the image, then the generated image will have a lot of space.

Yes I wanted to limit the selection space, but the example code was rather complex. Maybe I will try again: https://fengyuanchen.github.io/cropperjs/v2/api/cropper-selection.html#limit-boundaries

@silverwind
Copy link
Member Author

silverwind commented Mar 20, 2025

Maybe https://github.com/github/image-crop-element is also worth investigation as a replacement for cropperjs. It seems like a lightweight alternative and is what GitHub also uses. The canvas interactions to actually crop the image need to performed in our code with that lib.

# for free to join this conversation on GitHub. Already have an account? # 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants