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

feat: file upload notification for datasets #1247

Merged
merged 5 commits into from
Feb 26, 2021
Merged

Conversation

vfried
Copy link
Contributor

@vfried vfried commented Feb 17, 2021

When all files finished uploading on a dataset add/edit

image

When a file failed

image

/deploy

Closes #1150

@vfried vfried requested a review from a team as a code owner February 17, 2021 12:57
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 17, 2021 12:59 Inactive
@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-1247.dev.renku.ch

Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

All good! Just a note on the text for the successful operation.

For the user, it's not clear he can move away from the page while uploading. I was tempted to suggest adding such a message, but probably that would just clog the UI even more. We could consider this later if we think it's useful.

@vfried vfried temporarily deployed to renku-ci-ui-1247 February 18, 2021 10:59 Inactive
@ciyer
Copy link
Contributor

ciyer commented Feb 22, 2021

I suggest the following:

  • If the file is greater than some configurable threshold (default 50 MB), inform the user that they will be notified when the upload finishes.
    image

@vfried vfried temporarily deployed to renku-ci-ui-1247 February 22, 2021 13:52 Inactive
@vfried vfried requested a review from a team as a code owner February 22, 2021 15:01
@vfried
Copy link
Contributor Author

vfried commented Feb 22, 2021

I just pushed new changes, if the file is bigger than 100MB they will see the message. Otherwise, they won't.

image

@vfried vfried temporarily deployed to renku-ci-ui-1247 February 22, 2021 15:01 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 22, 2021 15:11 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 22, 2021 15:26 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 24, 2021 08:38 Inactive
ciyer
ciyer previously approved these changes Feb 24, 2021
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

This works very well! I tested uploading files to two different datasets at the same time, and that worked, the notifications were easy to follow.

Ready to merge.

🎉

client/run-telepresence.sh Outdated Show resolved Hide resolved
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 24, 2021 10:28 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

It works well 🎉
I suggest a minor change to simplify the "upload threshold" structure before merging -- see the comment inline

helm-chart/renku-ui/values.yaml Outdated Show resolved Hide resolved
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 26, 2021 10:14 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 26, 2021 10:22 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 26, 2021 10:34 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 26, 2021 11:06 Inactive
@vfried vfried temporarily deployed to renku-ci-ui-1247 February 26, 2021 11:53 Inactive
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

👍

@vfried vfried merged commit 406f3f4 into master Feb 26, 2021
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

@ciyer ciyer deleted the 1150-file-ds-notification branch April 1, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File upload notification
4 participants