-
Notifications
You must be signed in to change notification settings - Fork 21
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
[enhancement] upload progress fixes #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits.
client/src/components/Upload.vue
Outdated
item => item.status !== "done" && item.status !== "error" | ||
).length; | ||
}, | ||
// Takes the pending upload and returns the # of images or size of the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For comments like this, it may make sense to just write a full jsdoc comment. You've gone to the trouble of documenting this functions's signature in English, so I might as well get some type hints out of it.
client/src/components/Upload.vue
Outdated
}, | ||
// Takes the pending upload and returns the # of images or size of the file | ||
computeUploadProgress(pendingUpload) { | ||
// formatSize, totalProgress and totalSize are located in the fileUploader and sizeFormatter mixin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// formatSize, totalProgress and totalSize are located in the fileUploader and sizeFormatter mixin | |
const { formatSize, totalProgress, totalSize } = this; // use methods and properties from mixins |
IMO this line is better because the code tells me what is being used, and the comment only needs to tell me where it came from. I'm not going to remember to update this comment if the method changes.
client/src/components/Upload.vue
Outdated
this.$emit("update:uploading", false); | ||
console.log(uploaded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console log.
client/src/components/Upload.vue
Outdated
// Upload Mixin function to start uploading | ||
await this.start({ | ||
dest: folder, | ||
postUpload: postUpload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postUpload: postUpload | |
postUpload, |
{{ pendingUpload.files.length }} images | ||
<v-list-item-subtitle> | ||
{{ computeUploadProgress(pendingUpload) }} | ||
<!-- errorMessage is provided by the fileUploader mixin --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate this, I think it's a lots cause. I've never seen anyone comment every time they use a mixin feature. Is this considered a best practice, or just something you like to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's something I do just for ease of use and readability if I ever have to look at it again. Is there an extension for VS code or some setup that will goTo a function that is imported as a mixin from a node_module. If I came at this with standard VS code setup and ctrl+shft+F for the function name I wouldn't be able to find it without turning on searching inside of node_modules. Then if I do something like 'errorMessage' is found 400+ times. This is the part where I really wish there was some namespacing for mixins, or even a naming standard mixinErrorMessage. I can obviously take some of them out.
client/src/components/Upload.vue
Outdated
this.$emit("update:uploading", false); | ||
this.$emit("uploaded", uploaded); | ||
}, | ||
async uploadPending(pendingUpload, uploaded) { | ||
var { name, files, fps } = pendingUpload; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var { name, files, fps } = pendingUpload; | |
let { name, files, fps } = pendingUpload; |
client/src/components/Upload.vue
Outdated
var { name, files, fps } = pendingUpload; | ||
fps = parseInt(fps); | ||
pendingUpload.uploading = true; | ||
var { data: folder } = await this.girderRest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var { data: folder } = await this.girderRest | |
let { data: folder } = await this.girderRest |
client/src/components/Upload.vue
Outdated
const postUpload = data => { | ||
uploaded.push({ | ||
folder, | ||
results: data.results, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check here? When running locally, everytime I try to upload, it hands with an error saying:
TypeError: Cannot read property 'results' of undefined
Does this happen for anyone else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not getting that error, but there should be a check there anyways. Was your girder web components updated in the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not upgrade GWC. I'll upgrade and report back with the results.
client/src/components/Upload.vue
Outdated
.catch(error => { | ||
if ( | ||
error.response && | ||
error.response.data && | ||
error.response.data.message | ||
) { | ||
this.errorMessage = error.response.data.message; | ||
} else { | ||
this.errorMessage = error; | ||
} | ||
pendingUpload.uploading = false; | ||
// Return empty object for the folder destructuring | ||
return { data: null }; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're already awaitng the girderRest call, can't this can be handled with a try-catch
around the await? This would make it easier to read as well as more standard for using await.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that, but I think it prevents the assignment destructuring of the returned value from the await.
let { data: folder } = await this.girderRest.post(......
The options I can think of are either to wrap the entire data and where it used inside the try statement (including other awaits), hoist the variable declaration with a 'var' (don't want to do) or declare the variable outside of the try block and then grab the data afterwards using assignment without declaration syntax (essentially wrapping the entire await in ())
let folder; try { ({ data: folder } = await this.girderRest.post(.......)
let me know if you have another method that I might not be thinking of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In response to that code snippet, I think that'd be fine, or at least better than the alternative. If you didn't want to use the weird destructuring assignment syntax, you could also do this:
let folder;
try {
const { data } = await this.girderRest.post(
...
);
folder = data;
}
And in the catch block you'd probably do something like
catch {
...
folder = null;
}
client/src/components/Upload.vue
Outdated
/** | ||
* Takes the pending upload and returns the # of images or size of the file based on the type and state | ||
* @param {Object} pendingUpload - contains , status, type | ||
* size, and list of files to upload. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* Takes the pending upload and returns the # of images or size of the file based on the type and state | |
* @param {Object} pendingUpload - contains , status, type | |
* size, and list of files to upload. | |
*/ | |
/** | |
* @param {{ type: string, size: number, files: Array, uploading: boolean }} pendingUpload | |
* @returns {number} # of images or size of file depending on type and state | |
*/ |
I noticed that your comment was out of date already. pendingUpload.status
was unused and pendingUpload.uploading
wasn't mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final comment, otherwise lgtm.
@@ -41,7 +41,8 @@ export default { | |||
}, | |||
/** | |||
* Takes the pending upload and returns the # of images or size of the file based on the type and state | |||
* @param {Object} pendingUpload - contains , status, type | |||
* @param {{ type: string, size: number, files: Array, uploading: boolean }} pendingUpload | |||
* @returns {number} # of images or size of file depending on type and state | |||
* size, and list of files to upload. | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 46 should be removed.
line 43 is duplicate information and should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, not thinking properly this morning.
Changes