-
Notifications
You must be signed in to change notification settings - Fork 223
Revamp content file mgt #4872
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
Revamp content file mgt #4872
Conversation
contentcuration/contentcuration/frontend/channelEdit/components/edit/EditModal.vue
Outdated
Show resolved
Hide resolved
946addc
to
0a2db6f
Compare
contentcuration/contentcuration/frontend/channelEdit/views/files/FileUpload.vue
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/views/files/FileUploadItem.vue
Show resolved
Hide resolved
@akolson Great work here. I just left a few comments |
Just checking in: I recall you doing some research on files. Are there more changes coming or should I finalize my review? |
57cfadc
to
f85945d
Compare
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.
Primary requested change is the download behavior. Please note, the mentioned approach won't work correctly in development because there's a redirect involved
this.fileUploadId = fileUpload.id; | ||
}, | ||
downloadFile() { | ||
window.open(this.fileDisplay.url, '_blank'); |
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.
contentcuration/contentcuration/frontend/shared/leUtils/FormatPresets.js
Outdated
Show resolved
Hide resolved
this.selectFirstFile(); | ||
if (this.primaryFileCount > 1 && !this.isRemoveFileApproved) { | ||
this.showRemoveFileWarning = true; | ||
} 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.
This else branch confuses me a bit. If there's only one file, we delete the file without approval?
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.
@bjester yes, correct. The approval is only for removal of the extra files. Also, based on the current logic, a user is unable to remove a single file, they can only replace or download it, so I think that should cover for cases where a user tries to remove a singly uploaded 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.
That really seems backwards to me. If the user removes the last remaining file, then the node becomes incomplete. Therefore it would seem to make sense that it would confirm the deletion of a sole file. The link you shared is a reference to your new /changed code. The existing logic appears to allow removal when there are more than one file, so in that sense, it was most defensive when there was only one file, whereas your logic is defensive (requiring approval) when there are more than one and very lax with one 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.
Ahh I get your point! The current implementation doesn't allow removal of single files as the delete option isn't displayed in these instances.
Maybe a more general question would be, do we want users deleting single files? If so, then it makes total sense to add another approval dialog (different from the approval for users that are removing extra files that we are moving away from) to warn users of the implications?
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.
Okay so it seems worthwhile to me to align this logic with that. In other words, have it check if file could be removed and if not, do nothing. As it's written, I feel it's confusing considering this behavior and aforementioned pathway
fdc4c61
to
de9591b
Compare
# Conflicts: # contentcuration/contentcuration/frontend/channelEdit/views/files/FileUploadItem.vue
25de86c
to
87abe3c
Compare
contentcuration/contentcuration/frontend/shared/vuex/file/actions.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/vuex/file/actions.js
Outdated
Show resolved
Hide resolved
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.
Seems pretty stable now. Thanks for handling this rabbit hole of a feature, @akolson !
Summary
This pr revamps tthe content file management UI as described in the figma designs. It also reorders the files display (although this is short-lived based on the next point). Lastly, it prevents uploading of multiple files to document resources. Users with existing multiple uploads will continue to work unaffected, however when they try to remove any of the uploads, they will be presented with a warning, as removing them means they can't add them back. This change means that no more multiple file types can be uploaded for a single resource.
Before:

After:

Old resources with more than one file uploaded;
New resources uploaded;

References
Closes #4840
Reviewer guidance
Note:
While running on your local, you might encounter this compile error particularly when interacting with uploaded epub files. Refreshing your browser should resolve the issue temporarily.