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

Improve upload UI #5081

Merged
merged 31 commits into from
Feb 16, 2021
Merged

Improve upload UI #5081

merged 31 commits into from
Feb 16, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jan 22, 2021

This PR reworks the Upload UI and additionally adds the check, whether a dataset needs to be converted to the front end. Therefore the user only has to set the dataset scale if necessary.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test (local):

  • Open the dataset upload ui via the dashboard.

  • Upload a new dataset in zip format.

  • Only if multiple datastores are available, the user can select a datastore to which the dataset should be uploaded.

  • Additionally, the user should only be required to enter the dataset scale, if the format of the dataset is not wkw.

  • Upon starting the upload a modal should open up, displaying the uploading process or errors.

  • After the upload finished, the content of the modal should change and suggest the user to either view the dataset, go back to the dashboard or go to the settings of the uploaded dataset. Each of those links should work as expected.

  • Additionally, after the upload finished successfully the modal should now be closable by using the ok button that was previously disabled.

  • Another to test is whether the checking of the zip file content for a wkw file takes too much memory. In my test cases, this never was a problem.

Issues:


@MichaelBuessemeyer
Copy link
Contributor Author

While working on this issue I came across the following question(s):
The dataset_upload_view is used twice in the project. Once in the onboarding and second in the "normal" upload view.
Prior to this PR, the dataset_upload_viewsimply called a method from the parent upon successful upload. Therefore the behavior after a successful upload was not consistent:

  • In the onboarding case, the modal in which the upload view was rendered, the settings of the just uploaded dataset are displayed. (And no conversion job is started if the dataset needed conversion).
  • In the "normal" upload view case, the user was redirected to the long-running jobs view if the uploaded dataset needs to be converted (at least that's what the code reads like) or to the settings of the just uploaded dataset.

Why isn't the case that the dataset needs a conversion included in the onboarding case?

Additionally, why do we have the

const handleDatasetAdded = async (
organization: string,
datasetName: string,
needsConversion: ?boolean,
scale: ?Vector3,
): Promise<void> => {
if (needsConversion && scale) {
await startJob(datasetName, organization, scale);
const url = "/jobs";
history.push(url);
} else {
const url = `/datasets/${organization}/${datasetName}/import`;
history.push(url);
}
};
method that triggers a conversion job? I the master it looks like this is already done automatically after the upload (when looking at the code):
let tooltip;
if (isFinished) {
tooltip = "Converting Dataset...";
} else {
tooltip = `${isRetrying ? "Retrying... - " : ""}${Math.round(
uploadProgress * 100,
)}% completed`;
}
.

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Could you please check out the new "reordered" UI?
Looking at the code should not be necessary for now as the first visual feedback is more important and the code is not tidied up yet.

@philippotto
Copy link
Member

Awesome stuff! Glad you got the lightweight wkw-detection within the zip to work 👍

The changes look very good to me overall, too. I only have some slight remarks:

  • I think, for the dev server, the job-feature is not enabled. In that case, the UI doesn't really give any hints in case the input needs conversion (since that would fail). There should be a message à la: "The selected dataset does not seem to be in the WKW format. Please convert the dataset using webknossos-cuber or use a webKnossos instance which integrates a conversion service, such as webknossos.org".
  • If one uploads an unsupported file format or a broken zip, there is no indication about this. Initially, I created a tar and named that zip (:facepalm:) and wondered why the code didn't traverse the zipped files. The console showed File format is not recognized., but the UI didn't say anything.
  • The UI changes look very good 👍 I only have a few suggestions:
    • there are two different folder icons in the UI, now. I'd prefer if we could use the icon from the modal in both places (the other one was slightly blurry for me due to the diagonal lines).
    • I'm digging the gradient-progressbar! However, I think we have other places were we use the plain-blue version of the progress bar. For consistency, I'd stick to that, I think 🤔
    • I think, it would look better if the actual modal-content (icon and "uploading" text) would be centered. Also, we could get rid of the modal-header, as it doesn't provide any useful information (and the close-button should be disabled probably, anyway).

image

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Jan 25, 2021

  • I think, for the dev server, the job-feature is not enabled. In that case, the UI doesn't really give any hints in case the input needs conversion (since that would fail). There should be a message à la: "The selected dataset does not seem to be in the WKW format. Please convert the dataset using webknossos-cuber or use a webKnossos instance which integrates a conversion service, such as webknossos.org".
  • done
  • If one uploads an unsupported file format or a broken zip, there is no indication about this. Initially, I created a tar and named that zip (:facepalm:) and wondered why the code didn't traverse the zipped files. The console showed File format is not recognized., but the UI didn't say anything.
  • done
  • there are two different folder icons in the UI, now. I'd prefer if we could use the icon from the modal in both places (the other one was slightly blurry for me due to the diagonal lines).

This is kinda tricky as our antd does not allow to render custom upload list items and only to set an image thumbnail url. Thus I copied the folder svg from antd into the folder.svg so we can simply set that file as the thumbnail.

  • done
  • I'm digging the gradient-progressbar! However, I think we have other places were we use the plain-blue version of the progress bar. For consistency, I'd stick to that, I think 🤔
  • done
  • I think, it would look better if the actual modal-content (icon and "uploading" text) would be centered. Also, we could get rid of the modal-header, as it doesn't provide any useful information (and the close-button should be disabled probably, anyway).
  • done

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto Could you please check out my questions above (first post in this PR)? 🛩️

@MichaelBuessemeyer
Copy link
Contributor Author

If one uploads an unsupported file format or a broken zip ...

The current code only checks for broken zip files and not for unallowed file types in the zip-folder.
@philippotto To check for valid files only in the zip, I need a list of supported files formats. Could you help me out here?
I guess that all formats that are allowed by wk-cuber are supported as long as jobs are enabled, right?
This would be a list of file ending with [wkw, tiff, jpg, png, bmp, ...]

@philippotto
Copy link
Member

If one uploads an unsupported file format or a broken zip ...

The current code only checks for broken zip files and not for unallowed file types in the zip-folder.
@philippotto To check for valid files only in the zip, I need a list of supported files formats. Could you help me out here?
I guess that all formats that are allowed by wk-cuber are supported as long as jobs are enabled, right?

Sorry, I wasn't really clear: I meant that the file type of the dropped file should be checked (not the content of the archive). For example, dropping a .tar archive is not supported (at least it didn't work for me) and the UI should simply say so that the user knows they should drop a zip file.

Regarding the actual archives content, it should be enough to simply watch out for wkw files. If there aren't any, we know that the cuber should be used (and that will decide which types are supported).

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Jan 29, 2021

In my local setup, I just noticed that in the initial onboarding no datastore is available and therefore a dataset cannot be uploaded.

  • TODO investigate this bug.

@philippotto
Copy link
Member

Great stuff! Looks quite good to me, but I have some design suggestions which I summarized in this screenshot:

image

Mainly, I changed the flexbox to column and adapted the font size, text and margins. Please also remove the empty footer (visible because it has a border). Also: shouldn't it be ... uploaded successfully ?

I achieved the screenshot by fiddling around with the HTML. The final HTML looked like this (should be easy to integrate into react).

<div style="display: flex;align-items: center;flex-direction: column;font-size: 20px;padding-top: 13px;">
   The dataset was successfully uploaded!
   <div class="centered-items" style="margin-top: 10px;font-size: 44px;"><button type="button" class="ant-btn ant-btn-primary"><span>View the Dataset</span></button><button type="button" class="ant-btn"><span>Open Dataset Settings</span></button><button type="button" class="ant-btn"><span>Go to Dashboard</span></button></div>
</div>```

@MichaelBuessemeyer
Copy link
Contributor Author

I just applied the feedback 🚀

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome! This feels really great 💯 🥇

I have one final suggestion regarding the "Allowed Teams" field, however, I'm not 100% sure about it. I'll create a thread in slack and ping you.

@fm3 fm3 mentioned this pull request Feb 9, 2021
24 tasks
@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I just implemented your suggestion: The default team of the organization the user is in will now be initially selected in the upload view.

Could you please also check this change?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome 🕺 🕺 🕺

@normanrz normanrz mentioned this pull request Feb 12, 2021
@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I just did some final testing and found a bug introduced by the auto-selection of the user's team on a demo instance.
The problem was that our code always expects that the team (even when having only selected one) are in an array. I just fixed that problem :)

Could you please give this a final check together with norman's changes? I do not want to have missed any cases that might crash the upload UI, as it is critical for new users. That would be very helpful.
If everything is alright, let's finally merge this 🥜

@philippotto
Copy link
Member

@philippotto I just did some final testing and found a bug introduced by the auto-selection of the user's team on a demo instance.
The problem was that our code always expects that the team (even when having only selected one) are in an array. I just fixed that problem :)

Good that you found this!

Could you please give this a final check together with norman's changes? I do not want to have missed any cases that might crash the upload UI, as it is critical for new users. That would be very helpful.
If everything is alright, let's finally merge this peanuts

I just gave it a final check and everything worked fine :) If this is deployed on wk.org, we do an additional check just to be sure.

While testing I noticed that there is some delay between the upload and the "whats-next-screen". The delay is caused because the back-end needs a second to do the "import" etc. I think, this is fine right now, but the UI said that the upload is at 100%, which could make the user think that nothing will happen now. I just pushed a small workaround for this by clamping the progress value to 99.9%. I think, that's fair for now. Do you agree? If so, feel free to merge & deploy :)

@MichaelBuessemeyer
Copy link
Contributor Author

If this is deployed on wk.org, we do an additional check just to be sure.

Thanks 🙏 😄

While testing I noticed that there is some delay between the upload and the "whats-next-screen". The delay is caused because the back-end needs a second to do the "import" etc.

Yeah that makes sense. I think your solution is fine.

Alternatively, we could change the text from "Uploading Dataset" to "Successfully Uploaded Dataset " and then add a second Progress bar below with the text "Importing dataset . This would make clear, that the uploading worked and the backend now works on the importing stuff.

I do not have a strong tendency to one of the solutions and I do not know how good my suggestions looks and whether it might be more confusion that explaining what going on to the user.

What do you think @philippotto?

@philippotto
Copy link
Member

Alternatively, we could change the text from "Uploading Dataset" to "Successfully Uploaded Dataset " and then add a second Progress bar below with the text "Importing dataset . This would make clear, that the uploading worked and the backend now works on the importing stuff.

I do not have a strong tendency to one of the solutions and I do not know how good my suggestions looks and whether it might be more confusion that explaining what going on to the user.

What do you think @philippotto?

Yes, I agree that this would be a bit better and more precise. I think I saw a hardcoded sleep of 3 seconds in the code. We could use that to update the new progressbar so that it goes from 0 to 100 in these 3 seconds. If you are up for it, feel free to implement this. However, if you don't have the time right now (i.e., this week), I'd suggest to merge the PR without it so that this can land in this week :)

@fm3
Copy link
Member

fm3 commented Feb 16, 2021

I’d encourage you to postpone small cosmetic changes like this one to a follow-up PR.
There will also likely be more changes to the upload process in coming weeks anyway (e.g. the option to select a directory without having to zip it). The exact messages etc may change again then.

@MichaelBuessemeyer MichaelBuessemeyer merged commit fa33c96 into master Feb 16, 2021
@philippotto philippotto deleted the improve-upload-ui branch June 14, 2022 11:37
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.

Improve Upload UI
4 participants