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 for deleting multiple directory message 47892 #47956

Merged

Conversation

@pradeepmurugesan
Copy link
Contributor

commented Apr 16, 2018

const message = distinctElements.length > 1 ? getConfirmMessage(nls.localize('confirmMoveTrashMessageMultiple', "Are you sure you want to delete the following {0} files?", distinctElements.length), distinctElements.map(e => e.resource))
: distinctElements[0].isDirectory ? nls.localize('confirmMoveTrashMessageFolder', "Are you sure you want to delete '{0}' and its contents?", distinctElements[0].name)
: nls.localize('confirmMoveTrashMessageFile', "Are you sure you want to delete '{0}'?", distinctElements[0].name);
const message = this.getMoveToTrashMessage(distinctElements);

This comment has been minimized.

Copy link
@bpasero

bpasero Apr 17, 2018

Member

@pradeepmurugesan it seems to me the same issue exists for the code below (after the comment "Confirm for deleting permanently") when bypassing the trash. Can we enrich your existing method getMoveToTrashMessage to also handle the case of deleting without trash? Then maybe it should be one method getDeleteMessage that has a parameter for wether the trash is used or not.

@bpasero bpasero added this to the April 2018 milestone Apr 17, 2018

@pradeepmurugesan pradeepmurugesan force-pushed the pradeepmurugesan:multiple-dir-delete-message branch from 3e1f76f to 816a21a Apr 17, 2018


if (distinctElements.length > 1) {
if (distinctElements[0].isDirectory) {
message = getConfirmMessage(nls.localize(`confirm${key}MessageMultipleDirectories`, messages[`confirm${key}MessageMultipleDirectories`], distinctElements.length), distinctElements.map(e => e.resource));

This comment has been minimized.

Copy link
@pradeepmurugesan

pradeepmurugesan Apr 17, 2018

Author Contributor

@bpasero I am not sure about interpolating a variable here. But I couldn't find any other easier way to remove the duplicate code, if used the keys as it is..

@bpasero
Copy link
Member

left a comment

@pradeepmurugesan even if it means more duplication, for our nls story we cannot have dynamic keys. So we need to stick with individual nls.localize() calls that have the string within. And I would not start to introduce a dictionary or any other helper.

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@bpasero In that case why can't we just have 2 methods separately to fetch the messages.. Having a flag and duplicating the if else logic is going to make the method huge and I am not seeing the advantage of having the flag.

kindly let me know if I am missing something

@bpasero

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@pradeepmurugesan sure 2 methods is also fine

@pradeepmurugesan pradeepmurugesan force-pushed the pradeepmurugesan:multiple-dir-delete-message branch 2 times, most recently from 71966e9 to 7289e8b Apr 18, 2018

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@bpasero Great thanks.. have updated.. Kindly review

@bpasero
Copy link
Member

left a comment

@pradeepmurugesan there is still an issue when the selection of files and folders is mixed, e.g. select a folder and a file and hit delete, the message wrongly indicates to me that the 2 things are both folders. I fear we have to distinguish that case as well or just talk about files/folders in the case of a multi selection if we do not want to be more specific.

@@ -725,6 +722,42 @@ class BaseDeleteFileAction extends BaseFileAction {
});
});
}

private getMoveToTrashMessage(distinctElements): string {

This comment has been minimized.

Copy link
@bpasero

bpasero Apr 19, 2018

Member

@pradeepmurugesan the distinctElements is missing a type annotation: distinctElements: ExplorerItem[]

This comment has been minimized.

Copy link
@pradeepmurugesan

pradeepmurugesan Apr 20, 2018

Author Contributor

fixed

return message;
}

private getDeleteMessage(distinctElements): string {

This comment has been minimized.

Copy link
@bpasero

bpasero Apr 19, 2018

Member

@pradeepmurugesan the distinctElements is missing a type annotation: distinctElements: ExplorerItem[]

This comment has been minimized.

Copy link
@pradeepmurugesan

pradeepmurugesan Apr 20, 2018

Author Contributor

fixed

@pradeepmurugesan pradeepmurugesan force-pushed the pradeepmurugesan:multiple-dir-delete-message branch from 7289e8b to 002d1dc Apr 20, 2018

@pradeepmurugesan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

@bpasero I didn't think about the Hybrid case earlier.. Fixed the same now. Kindly review

@bpasero bpasero modified the milestones: April 2018, May 2018 Apr 20, 2018

@bpasero

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

@isidorn if you want to merge for April, feel free to review. otherwise running out of time, moving to May.

@isidorn

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

@bpasero May is good

@bpasero bpasero modified the milestones: May 2018, April 2018 Apr 20, 2018

@bpasero

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Actually this is easy enough, thanks 🥂

@bpasero bpasero merged commit 97aaae1 into microsoft:master Apr 20, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.