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.

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

Jump to bottom

prevent comment on uploading #32263

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+26 −5
Open

prevent comment on uploading #32263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
992b0a0
prevent comment on uploading
sangcheol12 Oct 15, 2024
ddbefb7
add isUploding method in ComboMarkdownEditor
sangcheol12 Oct 15, 2024
d885025
prevent status button on uploading images
sangcheol12 Oct 15, 2024
e286eda
remove insert placeholder event handler after upload success
sangcheol12 Oct 15, 2024
7d13422
Add try-catch to handle potential upload errors in handleUploadFiles
sangcheol12 Oct 15, 2024
34cfdaf
disable modify button on image uploading
sangcheol12 Oct 15, 2024
f3c7c26
prevent upload button on uploading with callback
sangcheol12 Oct 22, 2024
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
5 changes: 5 additions & 0 deletions web_src/js/features/comp/ComboMarkdownEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ class ComboMarkdownEditor {
this.attachedDropzoneInst.emit(DropzoneCustomEventReloadFiles);
}

isUploading() {
if (!this.dropzone) return false;
return this.attachedDropzoneInst.getUploadingFiles().length !== 0;
}

setupTab() {
const tabs = this.container.querySelectorAll('.tabular.menu > .item');

Expand Down
8 changes: 5 additions & 3 deletions web_src/js/features/comp/EditorUpload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {

let uploadIdCounter = 0;

function uploadFile(dropzoneEl, file) {
function uploadFile(dropzoneEl, file, placeholderCallback) {
return new Promise((resolve) => {
const curUploadId = uploadIdCounter++;
file._giteaUploadId = curUploadId;
Expand All @@ -23,6 +23,8 @@ function uploadFile(dropzoneEl, file) {
};
dropzoneInst.on(DropzoneCustomEventUploadDone, onUploadDone);
dropzoneInst.handleFiles([file]);
// if there is no setTimeout, then ComboMarkdownEditor.isUploading() does not working correctly
setTimeout(() => placeholderCallback(), 0);
});
}

Expand Down Expand Up @@ -99,8 +101,8 @@ async function handleUploadFiles(editor, dropzoneEl, files, e) {
const {width, dppx} = await imageInfo(file);
const placeholder = `[${name}](uploading ...)`;

editor.insertPlaceholder(placeholder);
await uploadFile(dropzoneEl, file); // the "file" will get its "uuid" during the upload
// the "file" will get its "uuid" during the upload
await uploadFile(dropzoneEl, file, () => editor.insertPlaceholder(placeholder));
Comment on lines +104 to +105

This comment was marked as outdated.

Sign in to view

Copy link
Contributor

@wxiaoguang wxiaoguang Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just think again about the problem. It looks that the setTimeout is somewhat hacky, and it doesn't guarantee that it would work with other uploaders.

I will try to make some new changes to introduce a separate event to handle this situation.

Copy link
Author

@sangcheol12 sangcheol12 Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this suggestion, third parameter (placeholder) seems not to be necessary. isn't it
but in that case, disabling submit button doesn't working correctly.

Copy link
Contributor

@wxiaoguang wxiaoguang Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, the first suggestion is to remove the placeholderCallback.

But indeed the setTimeout seems somewhat hacky, and if we'd like to improve the uploader or change it in the future, it might become a blocker (or regression).

So, a more general solution in my mind is: introduce a more general "opts.onUploadStateChanged" callback, make the ComboMarkdownEditor listen on Dropzone events and call the onUploadStateChanged, then UI code could sync the UI state accordingly. It could 100% isolate the business code and the uploader and be clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, one more thing ........ the current solution (onContentChanged) is also buggy when the user uploads a file by the Dropzone directly, there is no content change, then there is no "isUploading" check .....

Quote my previous comment:

I think the UI states should be managed by some well-defined events&handlers, but not by an ad-hoc "thumbnail" event with onContentChanged callback.

So, the problem is not that simple as it looks. I will take a look when I have time.

editor.replacePlaceholder(placeholder, generateMarkdownLinkForAttachment(file, {width, dppx}));
}
}
Expand Down
13 changes: 12 additions & 1 deletion web_src/js/features/repo-issue-edit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,21 @@ async function onEditContent(event) {

comboMarkdownEditor = getComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
if (!comboMarkdownEditor) {
const opts = {};
opts.onContentChanged = (editor) => {
const saveButton = editContentZone.querySelector('.ui.primary.button');
const isUploading = editor.isUploading();
if (saveButton) {
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
saveButton.disabled = isUploading;
}
};

editContentZone.innerHTML = document.querySelector('#issue-comment-editor-template').innerHTML;
comboMarkdownEditor = await initComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'));
comboMarkdownEditor = await initComboMarkdownEditor(editContentZone.querySelector('.combo-markdown-editor'), opts);

editContentZone.querySelector('.ui.cancel.button').addEventListener('click', cancelAndReset);
editContentZone.querySelector('.ui.primary.button').addEventListener('click', saveAndRefresh);
opts.onContentChanged(comboMarkdownEditor);
}

// Show write/preview tab and copy raw content as needed
Expand Down
5 changes: 4 additions & 1 deletion web_src/js/features/repo-issue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,11 +674,14 @@ export async function initSingleCommentEditor($commentForm) {
const commentButton = document.querySelector('#comment-button');
opts.onContentChanged = (editor) => {
const editorText = editor.value().trim();
const isUploading = editor.isUploading();

if (statusButton) {
statusButton.textContent = statusButton.getAttribute(editorText ? 'data-status-and-comment' : 'data-status');
statusButton.disabled = isUploading;
}
if (commentButton) {
commentButton.disabled = !editorText;
commentButton.disabled = !editorText || isUploading;
}
};
const editor = await initComboMarkdownEditor($commentForm.find('.combo-markdown-editor'), opts);
Expand Down