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

Confirm folder delete - fixes issue #4446 #4515

Merged
merged 8 commits into from Aug 1, 2013

Conversation

@g-palmer
Copy link
Contributor

commented Jul 19, 2013

Added confirm delete modal dialog when user deletes folder (not file) - issue #4446.

Passes lint and all tests. Have not added new tests (doesn't look like tests for file delete are in the DocumentCommandHandlers-test.js), but I'd love to add some!

Signed CLA.

Let me know if there's anything else I can do!

@@ -103,6 +103,8 @@ define({
"SAVE_CLOSE_MESSAGE" : "Do you want to save the changes you made in the document <span class='dialog-filename'>{0}</span>?",
"SAVE_CLOSE_MULTI_MESSAGE" : "Do you want to save your changes to the following files?",
"EXT_MODIFIED_TITLE" : "External Changes",
"CONFIRM_FOLDER_DELETE_TITLE" : "Confirm Delete",
"CONFIRM_FOLDER_DELETE" : "Are you sure you want to delete the folder?",

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Contributor

To show the file, use Are you sure you want to delete the folder <span class='dialog-filename'>{0}</span>? or similar. After using StringUtils.format, the folder name will replace {0} in the string.

{
className : Dialogs.DIALOG_BTN_CLASS_PRIMARY,
id : Dialogs.DIALOG_BTN_OK,
text : Strings.OK

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Contributor

@peterflynn Mentioned that he preferred Delete here instead of Ok. Maybe you could change this string. I am not sure if using CMD_FILE_DELETE will be ok, or if creating a new string for delete might be better.

]
)
.done(function (id) {
if (id === Dialogs.DIALOG_BTN_CANCEL) {

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Contributor

We probably don't need this branch of the if as it does nothing, and just use the other one.

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2013

This works great. Thanks for your work in this. I added a few comments around.

@g-palmer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2013

@TomMalbran Thanks for the quick feedback Tom! Made the requested changes.

Let me know if there's anything else.

Look forward to diving in deeper in the coming days!

@ghost ghost assigned TomMalbran Jul 19, 2013
@julianasuh

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2013

Assign to @TomMalbran

{
className : Dialogs.DIALOG_BTN_CLASS_PRIMARY,
id : Dialogs.DIALOG_BTN_OK,
text : Strings.CMD_FILE_DELETE

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 20, 2013

Contributor

Actually I prefer if this was a new string. If we ever change the name of the Command, this button should still be delete. Could you add the new string? Place it close to CANCEL and OK, as is the name of the button.

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2013

@g-palmer Thanks for the updates. I have one final comment. After that, we can merge, but since this adds new strings and we are past the UI is freeze. Will have to wait till the start of the next Sprint, around the 25th.

@g-palmer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2013

Made the change and added Strings.DELETE. I think this is what you were looking for.

Thanks again!

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2013

Yes. Thanks. Will have to wait for the next Sprint to merge as mentioned.

@g-palmer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2013

Yep. No worries. Glad it worked.

@TomMalbran

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2013

We are in Sprint 29, so we can merge this now :)

TomMalbran added a commit that referenced this pull request Aug 1, 2013
Confirm folder delete - fixes issue #4446
@TomMalbran TomMalbran merged commit f04c489 into adobe:master Aug 1, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@g-palmer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2013

Woohoo!

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.