gitea
gitea copied to clipboard
Refactor to use dropzone native methods upload files
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
I haven't got time to test it. Some questions:
-
this.element.parentElement.parentElement....
seems quite fragile. If there is a chance to make the code clear and stable, we should try. - 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. -
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. - 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. - 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.
I haven't got time to test it. Some questions:
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
- 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
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
- 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
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).
About uploading ... in editor Origin: https://user-images.githubusercontent.com/1255041/181421516-a0782b47-ce2e-40ff-a496-a5cb6cb8c181.mp4
New: https://user-images.githubusercontent.com/1255041/181421462-c4623c89-b811-4aff-bdc4-01dbcd958d40.mp4
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.
Add mouse cursor prompt to wait? Is it the solution?
$('.CodeMirror-lines').css('cursor', 'wait');
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.
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?
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.
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?
Thank you. The next steps in my mind:
- [x] A separate PR #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.
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.
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.
To fix https://github.com/go-gitea/gitea/issues/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).