Templates #2400
Templates #2400
Conversation
|
Think this is an ace addition to the app, making it super easy to make issues can only help productivity! Good work! |
Thanks! I really appreciate that |
|
Yessss super cool! |
|
Show/Stop spinner when fetching templates
|
Added completion block so the caller knows when the template UIAlertView is about to be displayed. This allows for showing a spin indicator in the calling view. I implemented that in the settings view. |
@ijm8710 if you could move this comment to #2393 this thread is more for the release of the feature, that thread is about the feature itself. Quick answer is, templates are a Github thing so we are mimicking the feature as much as possible. No fill ins etc. Just a template of what to write. Unfortunately that means deleting text. Sent with GitHawk |
- Fixed showing error when no template was found
- Moved regex to constants - Refactored regex matching to string extension
|
@Sherlouk Looking for a second review. If you could give some regex feedback I'd appreciate it. I'm pretty new to it. Maybe @BrianLitwin has some improvement suggestions on that. |
|
@rnystrom I think Bas pinged you on some reviews above. I believe the 2 topics were:
|
|
I haven't been following along b/c of the other eyeballs that have been on this. Lmk if/when I should give it a look. |
|
@rnystrom Any fixes required? |
|
Haven’t gotten to a review yet 😞 Sent with GitHawk |
|
|
||
| private let githubIssueURL = ".github/ISSUE_TEMPLATE" | ||
|
|
||
|
|
| // For Testing.. | ||
| if let testingFile = testingFile { completion(.success(testingFile)) } | ||
|
|
||
| self.fetchFile( |
| owner: repoDetails.repo.owner, | ||
| repo: repoDetails.repo.name, | ||
| branch: repoDetails.defaultBranch, | ||
| path: "\(githubIssueURL)/\(filename)") { (result) in |
| switch result { | ||
| case .success(let file): completion(.success(file)) | ||
| case .error(let error): completion(.error(error)) | ||
| case .nonUTF8: completion(.error(nil)) |
There was a problem hiding this comment.
nit: spacing inconsistent w/ rest of project. Newlines or single-space please
| testingFile: String? = nil, | ||
| completion: @escaping (Result<IssueTemplate>) -> Void | ||
| ) { | ||
| fetchTemplateFile(repoDetails: repo, filename: filename, testingFile: testingFile) { (result) in |
| with templates: [IssueTemplate], | ||
| details: IssueTemplateDetails) -> UIAlertController { | ||
|
|
||
|
|
|
|
||
|
|
||
| let alertView = UIAlertController( | ||
| title: NSLocalizedString("New Issue", comment: ""), |
| viewController.delegate = details.delegate | ||
| let navController = UINavigationController(rootViewController: viewController) | ||
| navController.modalPresentationStyle = .formSheet | ||
| details.viewController.route_present(to: navController) |
There was a problem hiding this comment.
router handles wrapping in nav controller
There was a problem hiding this comment.
@rnystrom Can you explain this a bit? If I switch to route_present(to: viewController) no navigation bar shows in the pushed view. I think I may not fully understand.
There was a problem hiding this comment.
Also a separate question: Should it be route_detail? on iPad it pushes the new VC over the entire screen, not into the detail section
There was a problem hiding this comment.
I see now that route_details wraps a nav controller if necessary. Cool stuff. Ill make this switch.
| viewController.delegate = details.delegate | ||
| let navController = UINavigationController(rootViewController: viewController) | ||
| navController.modalPresentationStyle = .formSheet | ||
| details.viewController.route_present(to: navController) |
There was a problem hiding this comment.
same note about nav controller, router handles wrapping
| """ | ||
|
|
||
| var cleanedFile = "" | ||
| if let textToClean = template.matches(regex: "([-]{3,})([\\s\\S]*)([-]{3,})").first { |
There was a problem hiding this comment.
Regex should be shared w/ actual implementation!
|
@rnystrom I'll take care of the nits and feedback. I replied above in regads to the number of networking calls. I don't think it can be reduced unfortuantly. |
|
@rnystrom Ready for another review whenever. Thanks for the last one! |
| import Squawk | ||
|
|
||
| private let githubIssueURL = ".github/ISSUE_TEMPLATE" | ||
| let tempalateRegex = ".github/ISSUE_TEMPLATE" |
| ) { | ||
|
|
||
| // For Testing. | ||
| if let testingFile = testingFile { completion(.success(testingFile)) } |
There was a problem hiding this comment.
Let's newline the closure execution here to stay consistent w/ other ifs
| } | ||
| } | ||
|
|
||
| func createTemplate( |
There was a problem hiding this comment.
nit: this needs a new name, I keep thinking that it's actually creating something. Can it just use the "fetch" naming used elsewhere?
| case .success(let files): | ||
| for file in files { | ||
| templateGroup.enter() | ||
| self.createTemplate(repoDetails: repoDetails, filename: file.name, completion: { result in |
There was a problem hiding this comment.
I'm going to block this PR on the multiple-requests part. It's just too wasteful IMO as-is. Some options:
- Do a fetch for the directory and just show the file names in the list instead of the template title. Then fetch the contents of the template once its been selected.
- Only fetch this info once then cache it. Always use the cache but allow the user to pull-to-refresh the templates list.
I favor option 1 personally.
There was a problem hiding this comment.
Sorry to jump in on here @rnystrom, and I know it's a massive shift in direction but could I suggest a third option here?
If we use the v4 API (ooh bois he's said the danger word) we can actually do this with a single request which returns the contents of every template within the directory?
This request: (Notably, we would need to replace the default branch and repo owner/name)
query {
repository(owner: "GitHawkApp", name:"GitHawk") {
object(expression: "master:.github/ISSUE_TEMPLATE") {
... on Tree {
entries {
name
object {
... on Blob {
text
}
}
}
}
}
}
}Returns us this response:
{
"data": {
"repository": {
"object": {
"entries": [
{
"name": "bug_report.md",
"object": {
"text": "---\nname: Bug report (etc etc, I've truncated this)"
}
},
{
"name": "feature_request.md",
"object": {
"text": "---\nname: Feature request (etc etc, I've truncated this)"
}
}
]
}
}
}
}One request, all the information? Opinion Ryan?
Sherlouk
added
😴 awaiting changes
Looking for preliminary feedback. I'm thinking of moving regex matching to an extension. I originally had it that the IssueTemplateHelper returned the templates to the caller and let it deal with it how it wished. I just then felt that since every view that wants to open a new issue needs to go through the same process that maybe its better to pass the view controller in and keep the code all in one place. Since it starts with an extension on the client the client isn't being passed around.
Anyways let me know what to fix.
@rnystrom @BrianLitwin @Sherlouk @BasThomas
TODO: