Fix for "Disallow Empty Bug Reports" Issue #2615 #2676
Conversation
Added new function in Squawk+GitHawk.swift, showIssueError, to handle error reporting through Squawk. The previous handling would display the error beneath the keyboard, on this form, and not be seen. Not that this should ever be seen. Added a function in NewIssueTableViewController.swift that checks both the UITextField and the UITextView for content before enabling the Submit button. After the Submit button is enabled and used, we will check once again to verify content, throwing visible errors via Squawk in the TextView if no text is present, before sending.
|
Hey I'm learning quite a bit about git lately. 😀 |
| @@ -205,6 +213,20 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| bodyField.inputAccessoryView = actions | |||
| } | |||
|
|
|||
| func updateSubmitButtonState() { | |||
| if titleText == nil { | |||
There was a problem hiding this comment.
Maybe combine these 2 ifs with an or ( ||) or all three with a ternary. Something like
navigationItem.rightBarButtonItem?.isEnabled = ( titleText == nil || bodyText == nil ) ? false : true
There was a problem hiding this comment.
Well the false : true is unnecessary, of course. Although you should then flip the result. 😊
There was a problem hiding this comment.
Hahahahah! Yes correct on both points. Totally forget the ternary
There was a problem hiding this comment.
Ok. I can change this around. How should I resubmit? I close and then do a new PR right?
There was a problem hiding this comment.
@wayni208 no need to close. Once you make a new commit to your branch it will update here
There was a problem hiding this comment.
There we go! Thanks for the feedback and direction on that function. Such a more elegant way to get it done.
There was a problem hiding this comment.
Awesome! Was about to say you don't have to reopen every time. 💯
| @@ -213,11 +235,18 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| return false | |||
| } | |||
|
|
|||
| // MARK: UITextViewDelegatre | |||
There was a problem hiding this comment.
nit: Spelling - UITextViewDelegate
| @@ -141,7 +144,12 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| /// Attempts to sends the current forms information to GitHub, on success will redirect the user to the new issue | |||
| @objc func onSend() { | |||
| guard let titleText = titleText else { | |||
| Squawk.showError(message: NSLocalizedString("You must provide a title!", comment: "Invalid title when sending new issue")) | |||
| Squawk.showIssueError(message: NSLocalizedString("You must provide a title!", comment: "Invalid title when sending new issue"), view: bodyField) | |||
There was a problem hiding this comment.
Thoughts on rewording this alert? Maybe something kinder like:
Title: "Title Required" or "Issue requires title"
Body: "Please provide a title and try again"
There was a problem hiding this comment.
Yep, I like title option 2... and I like the body. Mind updating that, @wayni208?
There was a problem hiding this comment.
Before I resolve this and because I had made the change before you replied @BasThomas , are we good on the message as I've written it? Here are both:
guard let titleText = titleText else {
Squawk.showIssueError(message: NSLocalizedString("An issue title is required. Please add a title and try again.", comment: "Invalid title when sending new issue"), view: bodyField)
return
}Also
cancelAction_onCancel(
texts: [titleText, bodyText],
title: NSLocalizedString("Unsaved Changes", comment: "New Issue - Cancel w/ Unsaved Changes Title"),
message: NSLocalizedString("Are you sure you want to discard this new issue? Your message will be lost.",
comment: "New Issue - Cancel w/ Unsaved Changes Message")
)Spelling error. Change in wording when Squawk error messages fire. Rewrote updateSubmitButtonState function to be more effective .
| @@ -205,6 +213,10 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| bodyField.inputAccessoryView = actions | |||
| } | |||
|
|
|||
| func updateSubmitButtonState() { | |||
| navigationItem.rightBarButtonItem?.isEnabled = ( titleText == nil || bodyText == nil ) ? false : true | |||
There was a problem hiding this comment.
Like @BasThomas mentioned, you actually can just leave it as
navigationItem.rightBarButtonItem?.isEnabled = !(titleText == nil || bodyText == nil)
There was a problem hiding this comment.
I see what you did there. 😀
Further revised updateSubmitButtonState function
| } | ||
|
|
||
| guard let bodyText = bodyText else { | ||
| Squawk.showIssueError(message: NSLocalizedString("Please provide as much of a detailed description possible and try again.", comment: "Invalid description when sending new issue"), view: bodyField) |
There was a problem hiding this comment.
Hm, I think this might be a bit confusing. Of course a good issue description is welcome, but not required. GitHub itself allows an empty body; I think we shouldn't fight that.
There was a problem hiding this comment.
How about we just throw up an "Are you sure you want to submit this issue with no description?" confirmation before send?
There was a problem hiding this comment.
I've made a few changes and reverted back to the original behavior where it would turn on the submit button once the titleField had changes. I've also added a confirmation dialogue notifying the user they have no description text and asking if they still want to submit or return to then form to add one.
I had to do a funky deal to get the dialogue correct. I split the onSend function in two because of the @objc function call in the accessibility button function and I needed another function to call on if the user decided to go ahead with the submit. I'm pretty certain this could have been done an easier way so please take a look and give me some direction if there is a better way to implement this.
There was a problem hiding this comment.
To be honest I'm still in favor of allowing an empty description.
There was a problem hiding this comment.
@BasThomas as in not popping an alert before sending an empty description?
| @@ -205,6 +213,10 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| bodyField.inputAccessoryView = actions | |||
| } | |||
|
|
|||
| func updateSubmitButtonState() { | |||
| navigationItem.rightBarButtonItem?.isEnabled = !( titleText == nil || bodyText == nil ) | |||
There was a problem hiding this comment.
I think we should split this up a bit, with the goal of making it easier to read (and again, not requiring a body).
What do you think about the following?
let hasTitleText = titleText != nil
let isTitleTextEmpty = titleText?.isEmpty == true // This would be additional. We could even check for whitespace only in the future if we want to.
navigationItem.rightBarButtonItem?.isEnabled = hasTitleText && isTitleTextEmpty == falseThere was a problem hiding this comment.
Higher up our titleText variable is checking for empty and whitespace.
private var titleText: String? {
guard let raw = titleField.text?.trimmingCharacters(in: .whitespacesAndNewlines) else { return nil }
if raw.isEmpty { return nil }
return raw
}I think the best option would be to revert to the original. If we are doing away with the UITextViewDelegate and not checking bodyField to turn on the Submit button we can get rid of that function completely.
@IBAction func titleFieldEditingChanged(_ sender: Any) {
navigationItem.rightBarButtonItem?.isEnabled = titleText != nil
}There was a problem hiding this comment.
Ah, I missed that. Go ahead.
| @@ -213,11 +225,18 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| return false | |||
| } | |||
|
|
|||
| // MARK: UITextViewDelegate | |||
|
|
|||
| /// Called when editing changed on the body field, enable/disable submit button based on title and body | |||
There was a problem hiding this comment.
Awesome you thought about the header docs! If we drop the body requirement though, this should be updated.
| @@ -205,6 +213,10 @@ final class NewIssueTableViewController: UITableViewController, UITextFieldDeleg | |||
| bodyField.inputAccessoryView = actions | |||
| } | |||
|
|
|||
| func updateSubmitButtonState() { | |||
There was a problem hiding this comment.
Would you like to write a test for this? :)
There was a problem hiding this comment.
We can do away with that function as I mentioned. But if we don't, I sure will!
There was a problem hiding this comment.
I'm concerned about having split that onSend function in two and if I need to write a test for it. Also concerned about writing the test. I couldn't see anywhere where we are currently testing this portion of the code. I'm unfamiliar with tests in general so if you could give me some direction on this I would appreciate it.
There was a problem hiding this comment.
I'll get back to you on this one once I have a bit more time. Let's leave it for now; it's OK :)
Reverted to original code for function titleFieldEditingChanged. Removed textViewDelegate and updateSubmitButtonState function which watched both titleField and bodyField for content before enabling the Submit button. Split onSend function in two separate functions, onSend and finishSend. onSend now looks for content in bodyField and if none is found instantiates an IAAlertController to notify the user that they are missing a description and giving them the option to send it anyway or return to the form and add one. finishSend completes the send.
Reverted to original code for function titleFieldEditingChanged. Removed textViewDelegate and updateSubmitButtonState function which watched both titleField and bodyField for content before enabling the Submit button. Split onSend function in two separate functions, onSend and finishSend. onSend now looks for content in bodyField and if none is found instantiates an IAAlertController to notify the user that they are missing a description and giving them the option to send it anyway or return to the form and add one. finishSend completes the send.
| if let bodyText = bodyText { | ||
| self.finishSend() | ||
| } else { | ||
| let submitAlert = UIAlertController(title: "Please Provide Description", message: "Are you certain you want to submit this issue without a description?", preferredStyle: UIAlertControllerStyle.alert) |
There was a problem hiding this comment.
You can just do .alert it will infer the type
Huddie
added
😴 awaiting changes
Cleaned up func onSend to remove redundancies
Huddie
added
💤 awaiting review
| } else { | ||
| let submitAlert = UIAlertController(title: "Please Provide Description", message: "Are you certain you want to submit this issue without a description?", preferredStyle: .alert) | ||
|
|
||
| submitAlert.addAction(UIAlertAction(title: "Submit", style: .default, handler: { _ in self.finishSend() })) |
There was a problem hiding this comment.
The indentation here is still a bit off.
(You can use ⌃+I to reindent a selection)
onSend method only guards against an empty titleField. If empty, calls Squawk.showIssueError to properly display error message in the bodyField above the keyboard.
Added new function in Squawk+GitHawk.swift, showIssueError, to handle error reporting through Squawk. The previous handling would display the error beneath the keyboard, on this form, and not be seen. Not that this should ever be seen.
Added a function in NewIssueTableViewController.swift that checks both the UITextField and the UITextView for content before enabling the Submit button. After the Submit button is enabled and used, we will check once again to verify content, throwing visible errors via Squawk in the TextView if no text is present, before sending.
Closes #2615