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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9f47b23
fixed #33321
kerwin612 Feb 5, 2025
2255362
fix
kerwin612 Feb 5, 2025
7ab8dc3
fix
kerwin612 Feb 5, 2025
546f133
refactor
wxiaoguang Feb 5, 2025
006dd61
fix
wxiaoguang Feb 5, 2025
b34ab60
fine tune prompt margin
wxiaoguang Feb 5, 2025
2609e92
Merge branch 'main' into fix_33321
wxiaoguang Feb 5, 2025
f045e77
introduce avatar-file-with-cropper again
wxiaoguang Feb 5, 2025
8065660
Merge branch 'main' into fix_33321
GiteaBot Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions templates/admin/user/edit.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,7 @@
</div>

<div class="inline field tw-pl-4">
<label for="avatar">{{ctx.Locale.Tr "settings.choose_new_avatar"}}</label>
<input name="avatar" type="file" accept="image/png,image/jpeg,image/gif,image/webp">
{{template "shared/avatar_upload_crop" dict "LabelText" (ctx.Locale.Tr "settings.choose_new_avatar")}}
</div>

<div class="field">
Expand Down
4 changes: 1 addition & 3 deletions templates/org/settings/options.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,8 @@
<form class="ui form" action="{{.Link}}/avatar" method="post" enctype="multipart/form-data">
{{.CsrfTokenHtml}}
<div class="inline field">
<label for="avatar">{{ctx.Locale.Tr "settings.choose_new_avatar"}}</label>
<input name="avatar" type="file" accept="image/png,image/jpeg,image/gif,image/webp">
{{template "shared/avatar_upload_crop" dict "LabelText" (ctx.Locale.Tr "settings.choose_new_avatar")}}
</div>

<div class="field">
<button class="ui primary button">{{ctx.Locale.Tr "settings.update_avatar"}}</button>
<button class="ui red button link-action" data-url="{{.Link}}/avatar/delete">{{ctx.Locale.Tr "settings.delete_current_avatar"}}</button>
Expand Down
3 changes: 1 addition & 2 deletions templates/repo/settings/options.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@
<form class="ui form" action="{{.Link}}/avatar" method="post" enctype="multipart/form-data">
{{.CsrfTokenHtml}}
<div class="inline field">
<label for="avatar">{{ctx.Locale.Tr "settings.choose_new_avatar"}}</label>
<input name="avatar" type="file" accept="image/png,image/jpeg,image/gif,image/webp">
{{template "shared/avatar_upload_crop" dict "LabelText" (ctx.Locale.Tr "settings.choose_new_avatar")}}
</div>
<div class="field">
<button class="ui primary button">{{ctx.Locale.Tr "settings.update_avatar"}}</button>
Expand Down
8 changes: 8 additions & 0 deletions templates/shared/avatar_upload_crop.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{{- /* we do not need to set for/id here, global aria init code will add them automatically */ -}}
<label>{{.LabelText}}</label>
<input name="avatar" type="file" accept="image/png,image/jpeg,image/gif,image/webp">
{{- /* the cropper-panel must be next sibling of the input "avatar" */ -}}
<div class="cropper-panel tw-hidden">
<div class="tw-my-2">{{ctx.Locale.Tr "settings.cropper_prompt"}}</div>
<div class="cropper-wrapper"><img class="cropper-source" src alt></div>
</div>
8 changes: 1 addition & 7 deletions templates/user/settings/profile.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,7 @@
</div>

<div class="inline field tw-pl-4">
<label for="new-avatar">{{ctx.Locale.Tr "settings.choose_new_avatar"}}</label>
<input id="new-avatar" name="avatar" type="file" accept="image/png,image/jpeg,image/gif,image/webp">
</div>

<div class="field tw-pl-4 cropper-panel tw-hidden">
<div>{{ctx.Locale.Tr "settings.cropper_prompt"}}</div>
<div class="cropper-wrapper"><img class="cropper-source" src alt></div>
{{template "shared/avatar_upload_crop" dict "LabelText" (ctx.Locale.Tr "settings.choose_new_avatar")}}
</div>

<div class="field">
Expand Down
2 changes: 1 addition & 1 deletion web_src/css/features/cropper.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@import "cropperjs/dist/cropper.css";

.page-content.user.profile .cropper-panel .cropper-wrapper {
input[name="avatar"] + .cropper-panel .cropper-wrapper {
max-width: 400px;
max-height: 400px;
}
3 changes: 3 additions & 0 deletions web_src/js/features/admin/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import $ from 'jquery';
import {checkAppUrl} from '../common-page.ts';
import {hideElem, showElem, toggleElem} from '../../utils/dom.ts';
import {POST} from '../../modules/fetch.ts';
import {initAvatarUploaderWithCropper} from '../comp/Cropper.ts';

const {appSubUrl} = window.config;

Expand Down Expand Up @@ -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 •
edited
Loading

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)

gitea/web_src/js/features/common-form.ts

Line 34 in 7e596bd

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

Copy link
Contributor

@wxiaoguang wxiaoguang Feb 5, 2025 •
edited
Loading

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

}
3 changes: 3 additions & 0 deletions web_src/js/features/common-organization.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {initCompLabelEdit} from './comp/LabelEdit.ts';
import {toggleElem} from '../utils/dom.ts';
import {initAvatarUploaderWithCropper} from './comp/Cropper.ts';

export function initCommonOrganization() {
if (!document.querySelectorAll('.organization').length) {
Expand All @@ -13,4 +14,6 @@ export function initCommonOrganization() {

// Labels
initCompLabelEdit('.page-content.organization.settings.labels');

initAvatarUploaderWithCropper(document.querySelector('.organization.settings.options input[name="avatar"]'));
}
10 changes: 9 additions & 1 deletion web_src/js/features/comp/Cropper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ type CropperOpts = {
fileInput: HTMLInputElement,
}

export async function initCompCropper({container, fileInput, imageSource}: CropperOpts) {
async function initCompCropper({container, fileInput, imageSource}: CropperOpts) {
const {default: Cropper} = await import(/* webpackChunkName: "cropperjs" */'cropperjs');
let currentFileName = '';
let currentFileLastModified = 0;
Expand Down Expand Up @@ -38,3 +38,11 @@ export async function initCompCropper({container, fileInput, imageSource}: Cropp
}
});
}

export async function initAvatarUploaderWithCropper(fileInput?: HTMLInputElement) {
if (!fileInput) return;
const panel = fileInput.nextElementSibling as HTMLElement;
if (!panel?.matches('.cropper-panel')) throw new Error('Missing cropper panel for avatar uploader');
const imageSource = panel.querySelector<HTMLImageElement>('.cropper-source');
await initCompCropper({container: panel, fileInput, imageSource});
}
3 changes: 3 additions & 0 deletions web_src/js/features/repo-settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {minimatch} from 'minimatch';
import {createMonaco} from './codeeditor.ts';
import {onInputDebounce, queryElems, toggleElem} from '../utils/dom.ts';
import {POST} from '../modules/fetch.ts';
import {initAvatarUploaderWithCropper} from './comp/Cropper.ts';
import {initRepoSettingsBranchesDrag} from './repo-settings-branches.ts';

const {appSubUrl, csrfToken} = window.config;
Expand Down Expand Up @@ -156,4 +157,6 @@ export function initRepoSettings() {
initRepoSettingsSearchTeamBox();
initRepoSettingsGitHook();
initRepoSettingsBranchesDrag();

initAvatarUploaderWithCropper(document.querySelector('.repository.settings.options input[name="avatar"]'));
}
11 changes: 2 additions & 9 deletions web_src/js/features/user-settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
import {hideElem, showElem} from '../utils/dom.ts';
import {initCompCropper} from './comp/Cropper.ts';

function initUserSettingsAvatarCropper() {
const fileInput = document.querySelector<HTMLInputElement>('#new-avatar');
const container = document.querySelector<HTMLElement>('.user.settings.profile .cropper-panel');
const imageSource = container.querySelector<HTMLImageElement>('.cropper-source');
initCompCropper({container, fileInput, imageSource});
}
import {initAvatarUploaderWithCropper} from './comp/Cropper.ts';

export function initUserSettings() {
if (!document.querySelector('.user.settings.profile')) return;

initUserSettingsAvatarCropper();
initAvatarUploaderWithCropper(document.querySelector('.user.settings.profile input[name="avatar"]'));

const usernameInput = document.querySelector<HTMLInputElement>('#username');
if (!usernameInput) return;
Expand Down
Loading