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

Feature/3423/show upload progress #3864

Merged
merged 10 commits into from Jul 27, 2020

Conversation

marcoambrosini
Copy link
Member

@marcoambrosini marcoambrosini commented Jul 1, 2020

package.json Outdated Show resolved Hide resolved
@marcoambrosini marcoambrosini force-pushed the feature/3423/show-upload-progress branch 3 times, most recently from 53ee0f3 to 3d02ce8 Compare July 15, 2020 04:19
@marcoambrosini marcoambrosini marked this pull request as ready for review July 15, 2020 04:20
@marcoambrosini marcoambrosini self-assigned this Jul 15, 2020
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
@danxuliu danxuliu force-pushed the feature/3423/show-upload-progress branch from a39dd6b to 6489b38 Compare July 24, 2020 11:35
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Please extract commit "Fix spinner position" to its own pull request for easier backporting, as the bug is present too in stable19 (I have not checked stable18). Also please fix (in that pull request is fine) the message date being shown in temporary messages if they are tall (to test just send a message that has several new lines, you will see that the temporary message shows "1:00 AM" as its date).

The temporary message for files being uploaded has a couple of issues:

  • When the temporary message is added the message list is not scrolled to the bottom.
  • When the upload finishes the temporary message is removed, and when the message list is received again from the server the uploaded file is shown, which causes some flickering. I guess that the file upload message sent by the server should include a referenceId parameter to solve this (and the explicit deletion of the temporary message after the file is uploaded should be removed), although I have not checked.

When a file is uploaded an Vue is built in development mode there is a validation error shown in the browser console, although I guess that it is fixed by nextcloud-libraries/nextcloud-vue#1218 and thus it will be gone once a new nextcloud-vue version is released. As it is just a validation issue in development mode I would be fine merging this pull request before that is fixed.

Finally I have also noticed that in Chromium it is not possible to upload the same file twice in a row, but it happens too in master, it is not related to this pull request. In Firefox it works fine. Fortunately it should not be a very common use case ;-)

src/store/fileUploadStore.js Outdated Show resolved Hide resolved
src/store/fileUploadStore.js Outdated Show resolved Hide resolved
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
@marcoambrosini marcoambrosini force-pushed the feature/3423/show-upload-progress branch from 2039dcd to b6f3487 Compare July 27, 2020 08:42
Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
@nickvergessen
Copy link
Member

When the upload finishes the temporary message is removed, and when the message list is received again from the server the uploaded file is shown, which causes some flickering. I guess that the file upload message sent by the server should include a referenceId parameter to solve this (and the explicit deletion of the temporary message after the file is uploaded should be removed), although I have not checked.

I will have a look if we can hack this somehow in on the share request on the backend. So we will do this as a follow up.

@nickvergessen nickvergessen merged commit 7cfbaae into master Jul 27, 2020
@nickvergessen nickvergessen deleted the feature/3423/show-upload-progress branch July 27, 2020 14:07
@nickvergessen
Copy link
Member

Reference id handled in #3948

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show progress bar during upload
3 participants