Use correct error message when renaming/deleting folder #8008
Conversation
| @@ -40,7 +40,9 @@ define({ | |||
| "UNSUPPORTED_ENCODING_ERR" : "{APP_NAME} currently only supports UTF-8 encoded text files.", | |||
| "FILE_EXISTS_ERR" : "The file or directory already exists.", | |||
| "FILE" : "file", | |||
| "FILE_TITLE" : "File", | |||
There was a problem hiding this comment.
Is this the key to use?
There was a problem hiding this comment.
Title Casing some but not all dialogs is probably not the way to do this which is why I've added a UI audit to the trello board. I don't think any of the dialogs in project manager use title case for the titles so I vote for leaving this as-is for now. This change makes the rename case different than the create case but we should merge those two handlers so we maintain consistent messaging. There are a few other errors that are not title-case in project manager. Care to fix them as well in this PR since you're cleaning things up? "Error Loading Project, etc..."
|
Since you are correcting the text before the file path, why not do the same for the "The file or directory already exists" part ( |
| @@ -1782,16 +1782,22 @@ define(function (require, exports, module) { | |||
| _redraw(true); | |||
| result.resolve(); | |||
| } else { | |||
| var titleType = isFolder ? Strings.DIRECTORY_TITLE : Strings.FILE_TITLE, | |||
There was a problem hiding this comment.
You should refactor this to use the errorcallback on line 1661 instead so we have the same error messages at least. We have a much better message in there anyway and the logic is much simpler since the message is more generic.
There was a problem hiding this comment.
You might just want to refactor the error handler so that it calls a common function for displaying the error and pass in an indicator for the operation so we know which strings to use ( Strings.ERROR_CREATING_FILE_TITLE / Strings.ERROR_CREATING_FILE for the create case and Strings.ERROR_RENAMING_FILE_TITLE / Strings.ERROR_RENAMING_FILE for the rename case. ) but
We should have similar messaging for both rename/create cases.
I wonder do you think it's useful to show the OS error string for the rename but not the create case? We might want to make the error more robust for create if you want to merge in the code that for showing the OS error information for create as well.
|
@le717 Because that's another case. If you already have a folder |
|
@JeffryBooher Changes pushed. |
| message; | ||
| path = StringUtils.breakableUrl(path); | ||
|
|
||
| if (context === "create") { |
There was a problem hiding this comment.
these should be constants as for JSDoc is concerned
/*
* @constant
* @type {string} ERR_CTX_CREATE some description
*/
var ERR_CTX_CREATE = "create";
|
@SAplayer nice job with the refactoring. just need to resolve another issue and fix the merge error |
|
@JeffryBooher Everything ready. I hope it's fine now. |
| if (error === FileSystemError.ALREADY_EXISTS) { | ||
| var titleType = isFolder ? Strings.DIRECTORY_NAME : Strings.FILENAME; |
There was a problem hiding this comment.
Can't you use the _showErrorDialog here and just pass in the Strings.ENTRY_WITH_SAME_NAME_EXISTS for message? The replacement format stuff will just be ignored since the string has no {0}, etc...
|
Done with review. Looks good just a few cleanup items to consider. |
|
@JeffryBooher Everything done. |
| title = Strings.ERROR_LOADING_PROJECT; | ||
| message = StringUtils.format(Strings.OPEN_DIALOG_ERROR, error); | ||
| } else if (errType === ERR_TYPE_INVALID_FILENAME) { | ||
| title = StringUtils.format(Strings.INVALID_FILENAME_TITLE, isFolder ? Strings.DIRECTORY_NAME : Strings.FILENAME); |
There was a problem hiding this comment.
This is still lowercase "Invalid filename" and "Invalid directory name". It doesn't look like DIRECTORY_NAME / FILENAME strings are used elsewhere so you should be able to modify them but double check that I didn't miss anything.
There was a problem hiding this comment.
Ahh, I missed those. Fixed.
|
@JeffryBooher Oh well, all these nits... Changed again. |
|
@SAplayer i think this was the final one :) |
|
Looks good! Merging |
Use correct error message when renaming/deleting folder
This is for #8003.