Feature/ui save document#32
Conversation
| } else { | ||
| closeCurrentDocument() | ||
| } | ||
|
|
There was a problem hiding this comment.
please remove blank lines at the end of a method
| } else { | ||
| self.dismiss(animated: true, completion: nil) | ||
| } | ||
|
|
| return | ||
| } | ||
|
|
||
| doc.revert(toContentsOf: doc.fileURL, completionHandler: nil) |
There was a problem hiding this comment.
need to look into why this is working. I don't get how this could work...
| } | ||
|
|
||
| func saveContent() { | ||
| if let URL = document?.fileURL { |
There was a problem hiding this comment.
would a guard be more appropriate (and readable) in this case?
| } | ||
|
|
||
| func saveContent() { | ||
| if let URL = document?.fileURL { |
There was a problem hiding this comment.
lowercase variable name please
|
|
||
| func saveContent() { | ||
| if let URL = document?.fileURL { | ||
| document?.save(to: URL, for: .forOverwriting) { success in |
There was a problem hiding this comment.
does this use the correct save-method? (i.e. goes back to CoreWrapper.backTranslate)
| } else { | ||
| self.showToast(controller: self, message: "Error while saving", seconds: 1, color: .red) | ||
| } | ||
|
|
| } | ||
|
|
||
| } | ||
|
|
|
|
||
| completion?() | ||
| } | ||
|
|
|
Code looks good, thank you! Only very minor things where we have to agree on best practice... Also needs some debugging of me / us to make sure everything is working as expected (see my comment on "document.save") |
|
Thanks for the clarifications and fixes! A few more things before we can merge and release it:
General question: is the color of the alert toast if saving failed the standard color for such toasts? It really doesn't look like native iOS to me... I might be wrong though. |
|
@TomTasche |
|
As far as I can tell we are not blocking the main thread anyway because we're using asyncAfter? Changes look good, will merge and release tomorrow! |
|
i mean from user perspective: during these > 1 sec he can not interact with the app, what in my opinion can have a negative effect on user experience. |
|
Oh I see, didn't know that. 1.5s should be fine then |
|
FYI, made one more followup commit: 84f4939 Pushing release to App Store now. |
2 participants
No description provided.