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

Fix drag and drop upload when a file is selected #7036

Merged
merged 1 commit into from
May 25, 2022

Conversation

JammingBen
Copy link
Contributor

Description

We've fixed a bug where uploading via drag and drop wouldn't work if one or more files were selected. This happened because the CreateAndUpload component was destroyed on a file selection.

This is the "easy" solution to the problem. Another, more complex solution would be to move all that upload logic from the CreateAndUpload component to somewhere else (useUploadHelpers composable e.g.) and initialize the Dropzone in file list directly. That would require quite some work though.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

@JammingBen JammingBen self-assigned this May 23, 2022
@JammingBen JammingBen marked this pull request as ready for review May 23, 2022 11:47
@JammingBen JammingBen requested a review from kulmann May 23, 2022 11:48
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Can't test properly with broken oCIS, trying with oc10 now. Do have some questions already

@@ -181,7 +181,9 @@ export default defineComponent({
},

showActions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was most likely involved in settings this up, but having a v-if on the wrapping div in a component seems like an antipattern versus having the parent component define whether the child should get rendered at all (I may miss the whole upload logic being handled here, maybe that needs to happen individually?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know 😞 Hence my note in the initial post, this is the "Easy-but-not-so-nice"-solution. Moving the upload logic is probably nicer but also way more complicated. But if we all decide to go this way, I'll rework this PR.

@@ -4,6 +4,7 @@
:has-bulk-actions="true"
:breadcrumbs="breadcrumbs"
:breadcrumbs-context-actions-items="[currentFolder]"
:show-actions-on-selection="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

How haven't we realized this wasn't the case before? 🤔

@@ -4,6 +4,7 @@
:has-bulk-actions="true"
:breadcrumbs="breadcrumbs"
:breadcrumbs-context-actions-items="[currentFolder]"
:show-actions-on-selection="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

How haven't we realized this wasn't the case before? 🤔

@@ -181,7 +181,9 @@ export default defineComponent({
},

showActions() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was most likely involved in settings this up, but having a v-if on the wrapping div in a component seems like an antipattern versus having the parent component define whether the child should get rendered at all (I may miss the whole upload logic being handled here, maybe that needs to happen individually?)

@JammingBen JammingBen force-pushed the drop-with-file-selection branch from d1ddf37 to 44bb716 Compare May 24, 2022 14:15
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

12.5% 12.5% Coverage
0.0% 0.0% Duplication

@JammingBen
Copy link
Contributor Author

As discussed in chat: I'm merging this with the "easy solution" for now. See #7062 for the follow-up.

@JammingBen JammingBen merged commit 7b2e63d into master May 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the drop-with-file-selection branch May 25, 2022 12:30
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.

Upload via Drag'n Drop doesn't work, if a resource is selected
2 participants