| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add drag and drop to move items in FileTreeView #13546
Conversation
Todo: * Handle move errors. * Add support for moving to root directory.
src/project/ProjectManager.js
Outdated
| var self = this; | ||
|
|
||
| // Remove selected marker | ||
| self.setSelected(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.
As of now, I just remove the selected flag if any of the items are moved. Need feedback on the correct way to handle it.
|
Really good stuff @boopeshmahendran! Some preliminary things:
|
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.
Some really small comments, really nice job 👍
src/project/ProjectManager.js
Outdated
| * See `ProjectModel.moveItem` | ||
| */ | ||
| ActionCreator.prototype.moveItem = function(oldPath, newDirectory) { | ||
| var self = this; |
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.
No need to set this to variable here (for now at least)
src/project/ProjectModel.js
Outdated
| // Reusing the _renameItem for moving item | ||
| this._renameItem(oldPath, fullNewPath, fileName, !_pathIsFile(fullNewPath)).then(function () { | ||
| var newDirectoryRelative = self.makeProjectRelativeIfPossible(newDirectory), | ||
| needsLoading = !self._viewModel.isPathLoaded(newDirectoryRelative); |
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.
Nit: some extra spaces after needsLoading
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.
Maybe save the reference to var viewModel = self._viewModel to make the calls a bit less terse.
src/project/ProjectModel.js
Outdated
| if (needsLoading) { | ||
| self._getDirectoryContents(newDirectory).then(function(contents) { | ||
| self._viewModel.setDirectoryContents(newDirectoryRelative, contents); | ||
| self._viewModel.moveItem(self.makeProjectRelativeIfPossible(oldPath), self.makeProjectRelativeIfPossible(fullNewPath)); |
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.
Maybe make these variables too self.makeProjectRelativeIfPossible(oldPath) self.makeProjectRelativeIfPossible(fullNewPath) as the long lines are pretty hard to distinquish.
|
@petetnt Thanks for reviewing. I'll address these comments. |
|
@petetnt I have addressed your comments. Please review. |
boopeshmahendran
requested review from
nethip,
petetnt,
ficristo,
swmitra,
shubhsnov and
navch
and removed request for
marcelgerber
src/filesystem/FileIndex.js
Outdated
|
|
||
| if (oldDirectory && oldDirectory._contents) { | ||
| oldDirectory._contents = oldDirectory._contents.filter(function(entry) { | ||
| if (entry.fullPath === oldPath || entry.fullPath === newPath) { |
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.
Why are we checking equality with both oldpath and newpath? Shouldn't it be just one of them?
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.
Yeah. Changed this.
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.
Codewise LGTM, didn't have the time to re-test the functionality yet
This makes adding the moved item to the new directory independent of whether the directory is loaded or not by creating a notFullyLoaded directory for the new directory.
|
Awesome work @boopeshmahendran 💯 Thanks for this PR! |
This PR adds the feature to drag and drop files and folders to another folder in the filetree to move them. We can drop items outside of all the folders to move items to the root folder.
I have tried to make the rename functions generic by making it accept new path instead of new name and reuse the rename workflow for the move item as there is a lot common between the two things.
Todo:-
@swmitra @nethip @petetnt @ficristo @navch @shubhsnov Please review