enable multi root DnD #45428
Merged
enable multi root DnD #45428
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@isidorn thanks will do |
bpasero
requested changes
Mar 10, 2018
| @@ -817,7 +817,7 @@ export class FileDragAndDrop extends SimpleFileResourceDragAndDrop { | |||
| return true; // NewStatPlaceholders can not be moved | |||
| } | |||
|
|
|||
| if (source.isRoot && (sources.length > 1 || target instanceof FileStat && !target.isRoot)) { | |||
| if (source.isRoot && target instanceof FileStat && !target.isRoot) { | |||
| return true; // Root folder can not be moved to a non root file stat. Do not allow root folder move when multi selection drag. | |||
There was a problem hiding this comment.
The comment needs an update too I think
| uri: folders[index].uri | ||
| }); | ||
| } | ||
| private doHandleRootDrop(roots: FileStat[], target: FileStat | Model): TPromise<void> { |
There was a problem hiding this comment.
Wouldn't it be much easier here to do something like:
- fill an array of roots that includes everything except the roots that are being dragged
- find the index where to drop to (if target is
model, the index is the length of the array) - add the roots that are being dragged into the array at the correct location
- call workspace editing service with that array
There was a problem hiding this comment.
Thanks for the suggestion. This simplifies a bit. This was tackled via 5564db9
bpasero
reviewed
Mar 10, 2018
| for (let index = 0; index < folders.length; index++) { | ||
| const data = { | ||
| name: folders[index].name, | ||
| uri: folders[index].uri |
There was a problem hiding this comment.
@isidorn also careful here, you are introducing a name property that previously might not have been there
There was a problem hiding this comment.
@bpasero how can I check if the name was there previously?
There was a problem hiding this comment.
@isidorn you don't need to, just leave name empty
|
Great, thanks for feedback. I feel like I have addressed all the issues -> merging |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #44554
Not the nicest code but gets the job done for multi select root DnD.
@bpasero please review and let me know if you know how to simplify this. Thanks!