Skip to content

Add cropping support when modifying the user/org/repo avatar #33498

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

Merged
merged 9 commits into from
Feb 6, 2025

Conversation

kerwin612
Copy link
Member

@kerwin612 kerwin612 commented Feb 5, 2025

Fixed #33321

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 5, 2025
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/frontend labels Feb 5, 2025
@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Feb 5, 2025
@lunny lunny changed the title fixed #33321 Fix missed cropping function when modifying the user avatar and repo avatar Feb 5, 2025
@lunny lunny added the type/bug label Feb 5, 2025
@lunny lunny added this to the 1.24.0 milestone Feb 5, 2025
@lunny lunny added the backport/v1.23 This PR should be backported to Gitea 1.23 label Feb 5, 2025
@pull-request-size pull-request-size bot removed the size/S label Feb 5, 2025
@kerwin612
Copy link
Member Author

repo:
1738738520112

org:
1738738618619

@wxiaoguang
Copy link
Contributor

When coding, need to do the best to avoid the unmaintainable code caused by copy-paste: 546f133

@wxiaoguang wxiaoguang added type/enhancement An improvement of existing functionality and removed backport/v1.23 This PR should be backported to Gitea 1.23 type/bug labels Feb 5, 2025
delvh
delvh previously requested changes Feb 5, 2025
@@ -258,4 +259,6 @@ export function initAdminCommon(): void {
window.location.href = this.getAttribute('data-redirect');
});
}

initAvatarUploaderWithCropper(document.querySelector('.admin.edit.user input[name="avatar"]'));
Copy link
Member

Choose a reason for hiding this comment

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

That looks like a really fragile selector to me.
Can we perhaps have a distinct js class instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it is the right thing to do.

If you want to introduce a general selector, it needs much more work to make it compatible with multiple elements on the same page, which is not our case.

The current approach should be good and clear enough, and it is easy to refactor in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think it's worth to introduce a general selector, feel free to try to propose some changes to see whether it would be simpler and more stable than this selector.

Copy link
Member

Choose a reason for hiding this comment

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

I mean…
As far as I can see, it's always the same action we perform.
So, wouldn't it make sense to invert the calls from each specific component takes care of initializing only its selector using the same method
to all elements matching this selector inside the document are initialized?
Or am I missing any case where we need additional logic?

If we don't need to customize the initialization of any avatar cropper (-> all avatar croppers are initialized exactly the same), then we can unify the logic.
Otherwise, I agree with you.
So, can you think of a case where we might want separate initialization logic or are already using it?

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 5, 2025

Choose a reason for hiding this comment

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

So, can you think of a case where we might want separate initialization logic or are already using it?

Yes, you are right, that's the key point.

Actually I have tried to use a general selector js-avatar-with-cropper (some traces were still left in 006dd61 🤣 ), but at last I decided to drop it because overall it looks stranger in this case ......

TBH at the moment I haven't got a real case, that's just my opinion that such selector should be flexible and clear enough. (The combo-markdown-editor has a similar case: auto-init or not .... but that's another story)

queryElems<HTMLElement>(document, '.combo-markdown-editor:not(.custom-init)', (el) => initComboMarkdownEditor(el));

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 5, 2025

Choose a reason for hiding this comment

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

By the way, I am planning some frontend changes to make the JS init system more stable. A brief idea is like this:

<button data-global-click="show-panel"></button>
<a data-global-click="link-acton"></a>
<div class="dropdown" data-global-init="initMyDropdown"></div>
<div class="editor" data-global-init="initComboMarkdownDropdown"></div>
<input name="avatar" data-global-init="initAvatarUploaderWithCropper">

Then we could resolve many longstanding problems:

  1. avoid many duplicate "init" calls (the case in this PR)
  2. incorrect init & double init (especially when use htmx or partial reloading)

But it is only a brief idea, just FYI

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 5, 2025
@GiteaBot GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Feb 5, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 5, 2025
@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 Feb 5, 2025
@wxiaoguang
Copy link
Contributor

@delvh managed to introduce avatar-file-with-cropper again in f045e77

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.

If no more suggestions, I will vote my approval

@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 Feb 6, 2025
@delvh delvh added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 6, 2025
@wxiaoguang wxiaoguang changed the title Fix missed cropping function when modifying the user avatar and repo avatar Add cropping support when modifying the user avatar and repo avatar Feb 6, 2025
@wxiaoguang wxiaoguang changed the title Add cropping support when modifying the user avatar and repo avatar Add cropping support when modifying the user/org/repo avatar Feb 6, 2025
@wxiaoguang wxiaoguang merged commit 40426ad into go-gitea:main Feb 6, 2025
26 checks passed
@wxiaoguang wxiaoguang deleted the fix_33321 branch February 6, 2025 13:07
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 6, 2025
LaoQi pushed a commit to LaoQi/gitea that referenced this pull request Feb 6, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 7, 2025
* giteaofficial/main:
  refactor: decouple context from migration structs (go-gitea#33399)
  Move gitgraph from modules to services layer (go-gitea#33527)
  Add go wrapper around git diff-tree --raw -r -M (go-gitea#33369)
  [skip ci] Updated translations via Crowdin
  Update MAINTAINERS (go-gitea#33529)
  Add cropping support when modifying the user/org/repo avatar (go-gitea#33498)
  [skip ci] Updated translations via Crowdin
  Add alphabetical project sorting (go-gitea#33504)
  Refactor gitdiff test (go-gitea#33507)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators May 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/templates This PR modifies the template files topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The administrator has no cropping function when modifying the user avatar
6 participants