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

Refactor to use dropzone native methods upload files #20263

Closed

Conversation

tyroneyeh
Copy link
Contributor

@tyroneyeh tyroneyeh commented Jul 6, 2022

Refer to #20147 to refactor the way to upload attachments and use dropzone to upload natively to achieve a better operating experience
For closes #20130 and #14291 and #14982

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 14, 2022

I haven't got time to test it. Some questions:

  1. this.element.parentElement.parentElement.... seems quite fragile. If there is a chance to make the code clear and stable, we should try.
  2. Do we still need the prompt uploading ... in the editor when pasting an image from clipboard? Usually users expect to see UI feedbacks when they press ^V. However if there are already a lot of files in dropzone, the UI feedback from dropzone may be not obvious, then the user may feel no feedback and press ^V multiple times.
  3. const startPos = editor.selectionStart || editor.getCursor && editor.getCursor('start'), endPos = editor.selectionEnd || editor.getCursor && editor.getCursor('end'), isimage = file.type.startsWith('image/') ? '!' : '', fileName = (isimage ? file.name.replace(/\.[^/.]+$/, '') : file.name); is too long to read or understand, I do not think it's worth to inline codes aggressively.
  4. The different editor's behavior is different, they could be processed separately, instead of using mixed (unclear) editor.selectionStart || editor.getCursor && editor.getCursor('start'), in case in the future the editor will be switched to another one.
  5. And datafiles = e.clipboardData && e.clipboardData.items || e.dataTransfer && e.dataTransfer.files; I also think the different events should be processed separately. Mixing them together makes it very difficult to change/test/refactor the code. Any small change may break something else.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 14, 2022
@tyroneyeh
Copy link
Contributor Author

I haven't got time to test it. Some questions:

  1. this.element.parentElement.parentElement.... seems quite fragile. If there is a chance to make the code clear and stable, we should try.

This section will only be executed if file.editor is not defined, this section is to ensure that file.editor is defined

  1. Do we still need the prompt uploading ... in the textarea when pasting an image from clipboard?

uploading ... do not need to be displayed in the textarea, because the upload progress has been displayed in the dropzone area below, and the image will be added to the textarea after uploading

  1. const startPos = editor.selectionStart || editor.getCursor && editor.getCursor('start'), endPos = editor.selectionEnd || editor.getCursor && editor.getCursor('end'), isimage = file.type.startsWith('image/') ? '!' : '', fileName = (isimage ? file.name.replace(/\.[^/.]+$/, '') : file.name); is too long to read or understand, I do not think it's worth to inline codes aggressively.

Adjusted

  1. The different editor's behavior is different, they could be processed separately, instead of using mixed (unclear) editor.selectionStart || editor.getCursor && editor.getCursor('start'), in case in the future the editor will be switched to another one.

I don't know how to separate, please help

Thanks

@wxiaoguang
Copy link
Contributor

About "2. Do we still need the prompt uploading ... in the editor when pasting an image from clipboard? ", I think the answer is still yes. For example, the issue editor's height can be very large (almost full screen), then if a user presses ^V to paste an image, the dropzone is out of the viewport, they will see no UI feedback.

Before, the CodeMirrorEditor and TextareaEditor are for such purpose, they are the abstraction layer for different editors to insert/replace placeholders.

Since a PR need 2 approvals to get merged, I am not sure whether there are enough people like this change. If anyone (maybe @silverwind ?) can help to review and get this PR approved in the future, I can help to improve this PR together (in the next few days).

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 28, 2022

I know the uploading will be shown in dropzone. But the problem is, the height of the editor can be very large, just try to press a lot of Enter new lines, the height will increase till to the nearly full screen.

Then you won't see the dropzone in viewport.

image

@tyroneyeh
Copy link
Contributor Author

Add mouse cursor prompt to wait? Is it the solution?
$('.CodeMirror-lines').css('cursor', 'wait');

@wxiaoguang
Copy link
Contributor

Add mouse cursor prompt to wait? Is it the solution? $('.CodeMirror-lines').css('cursor', 'wait');

Why not just use the placeholder in editor? IMO most editors have placeholders when uploading, including GitHub's editor.

@tyroneyeh
Copy link
Contributor Author

I don't like it very much myself placeholder in editor, and the uploading of pictures is very fast, and the issue of long text is not common.
What do other users think?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 29, 2022

But you can not assume every file is small or everyone's network is fast and stable.

Even worst, how about there is a network failure/error? Can users get a feedback from UI?

When writing a general purpose application for end users, all edge cases should be considered.

I am also open for the opinions from other users.

image

@tyroneyeh
Copy link
Contributor Author

The placeholder in the editor will only display uploading... and you can't know the status progress. It's more practical to pull down to see the status, what do you think?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 5, 2022

Thank you. The next steps in my mind:

  • A separate PR Move handleGlobalEnterQuickSubmit into a separate file to avoid cycle-import #20679 to reduce this PR's complexity and make this PR more focused.
  • After PR 20679 is merged, I will take a look at the relations between the editor - dropzone - (paste|drop) events to see whether it's possible to make the feature more maintainable.
  • Write some documents about the editor's test cases, how to fully test the editor's behaviors when the code is changed.

Update: I am quite busy recently, not sure when I have time to continue. Feel free to pick up or improve or approve.

@lunny lunny added topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Sep 22, 2022
@lunny lunny added this to the 1.19.0 milestone Oct 26, 2022
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Feb 2, 2023
@tyroneyeh tyroneyeh closed this Feb 17, 2023
@wxiaoguang
Copy link
Contributor

Hmm, sorry for seeing it's closed. If my review caused problems, I apologize, I also worked in my free time, and haven't got time thinking about the editor problems.

There are a lot of historical problems of Gitea's editor module (a lot of bugs and a lot of UI/UX problems), adding more patches may make it more difficult to maintain (and may cause more bugs ...)

If I have time in the future, I may try to refactor it and try to propose some other mechanisms.

Thank you very much.

@lunny lunny removed this from the 1.20.0 milestone Feb 17, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 2, 2023

After "Introduce GitHub markdown editor, keep EasyMDE as fallback #23876" , I'd like to restart the Dropzone related work.

#23876 introduces a general ComboMarkdownEditor, I think it would help to make the Dropzone related code simpler in the future, we can avoid many duplicate and fragile code like initEasyMDEFilePaste. (the second thought: "the relations between the editor - dropzone - (paste|drop) ... make the feature more maintainable" )

Thank you for your work again.

@silverwind
Copy link
Member

To fix #14982, we should define the textarea as a droppable area so image files can be dragged into. Essentially this needs to be done (Yes, HTML5 DnD API is weird).

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy and paste the images has list legend
5 participants