[WIP] Adds edit issue title #2348

BrianLitwin · 2018-10-23T22:33:50Z

Connected to #2054
See that thread for video and pics.. I did adjust the height to 120 as was recommended there.

Manually tested that editing the title updates three areas:

the issueTitle section controller
the timeline
the bookmark (when you go back to bookmarks, title is updated)

Included unit tests as well.
Used the viewerCanUpdate flag to determine editing privileges.
I also manually tested super long titles.. the textview handles them well and that shouldn't be an issue.

Update:

I realized this also needs to update the bookmark struct that the nav controller will use to set a new bookmark - eg currently, if you edit a title -> then bookmark the issue -> the bookmark will reference the original title, not the edited one.
@rnystrom Thanks for your review! I'm learning a lot. I'm going to update the Edit VC to follow a pattern more similar to People, Milestones, etc.. update everything optimistically and reset if there's a network failure. Was looking at the EditComment VC for cues, that was prob not the optimal apprach.
Updating the Bookmark API seems like a non-trivial issue, might need a separate PR, for the time being i'll do my best.

Sherlouk

Looks really good! Looking forward to this one

Classes/Issues/EditTitle/EditIssueTitleViewController.swift

Sherlouk · 2018-10-23T23:24:40Z

Local Pods/GitHubAPI/GitHubAPI/V3EditIssueTitleRequest.swift

+
+
+
+


nit: Bit of excessive spacing here

BrianLitwin · 2018-10-23T23:47:16Z

@Sherlouk Applied your suggestions.. thanks for the feedback!

BrianLitwin · 2018-10-24T16:36:11Z

Classes/Issues/IssuesViewController.swift

+        // assume the collectionview is pinned to the view's bounds
+        let contentInset = feed.collectionView.contentInset
+        return view.bounds.width - contentInset.left - contentInset.right
+    }


Moved this so that I could re-use it in the completion block after the fetch; see below

if let current = strongSelf.result { let issueResult = IssuesViewController.updateIssueResultModelTitle( newTitle: newTitle, oldTitle: strongSelf.currentIssueTitle ?? "", username: strongSelf.client.userSession?.username ?? Constants.Strings.unknown, issueResultModel: current, width: strongSelf.insetWidth ) strongSelf.client.cache.set(value: issueResult) }

Use localized string for title

BrianLitwin · 2018-10-27T18:40:40Z

@Sherlouk - I'm doing something wrong - I've been having trouble adding files to local pods, and getting the new XCode beta seemed to help, so I pulled this PR down to check on it, and the V3EditIssueTitleRequest in GitHubAPI didn't compile (didn't actually show up in XCode at all) - so I amended the commit, re-added the file so that it compiled and pushed it back up, and now there's this huge diff in Pods/Pods.xcodeproj/project.pbxproj and the check is failing.

For starters, I don't actually know the correct way to add a file to a local pod. When I try to add the file, it says Group: "GitHubAPI", and for target I'm putting a checkmark next to GithubAPI-iOS, because XCode doesn't complain and everything compiles when I do that. But I'm not sure that's correct?

rnystrom · 2018-10-27T19:24:05Z

Classes/Issues/IssuesViewController.swift

+    }
+
+    var viewerCanUpdate: Bool {
+        return result?.viewerCanUpdate ?? false


IssueManagingContextController already has access to IssueResult. These don't have to be delegate methods right? Can't these values just be fetched from the model when the edit VC is initialized?

rnystrom · 2018-10-27T19:24:49Z

Classes/Issues/EditTitle/EditIssueTitleViewController.swift

+protocol EditIssueTitleViewControllerDelegate: class {
+    func sendEditTitleRequest(newTitle: String, viewController: EditIssueTitleViewController)
+    var currentIssueTitle: String? { get }
+    var viewerCanUpdate: Bool { get }


See my notes about IssueManagingContextController already having access to the model. IMO these last 2 properties can be removed.

rnystrom · 2018-10-27T19:25:49Z

Classes/Issues/EditTitle/EditIssueTitleViewController.swift

+import SnapKit
+
+protocol EditIssueTitleViewControllerDelegate: class {
+    func sendEditTitleRequest(newTitle: String, viewController: EditIssueTitleViewController)


nit: delegate pattern has self param first, then other data params. Something like this would be more consistent:

func sendEditTitleRequest(from viewController: EditIssueTitleViewController, with title: String)

rnystrom · 2018-10-27T19:25:54Z

Classes/Issues/EditTitle/EditIssueTitleViewController.swift

+    var viewerCanUpdate: Bool { get }
+}
+
+class EditIssueTitleViewController: UIViewController {


rnystrom · 2018-10-27T19:27:07Z

Classes/Issues/EditTitle/EditIssueTitleViewController.swift

+            width: Styles.Sizes.contextMenuSize.width,
+            height: 120
+        )
+        title = NSLocalizedString("Edit", comment: "")


String "Edit" already exists, mind tossing that in Constants.swift?

rnystrom · 2018-10-27T19:35:19Z

Classes/Issues/IssuesViewController.swift

@@ -286,6 +288,13 @@ final class IssuesViewController: MessageViewController,
        activityController.popoverPresentationController?.barButtonItem = sender
        present(activityController, animated: trueUnlessReduceMotionEnabled)
    }
+
+    var insetWidth: CGFloat {


rnystrom · 2018-10-27T19:35:46Z

Classes/Issues/IssuesViewController.swift

-        // assume the collectionview is pinned to the view's bounds
-        let contentInset = feed.collectionView.contentInset
-        let width = view.bounds.width - contentInset.left - contentInset.right
+        let width = insetWidth


Just move insetWidth to the param? Doesn't need a let now right?

rnystrom · 2018-10-27T19:36:32Z

Classes/Issues/IssuesViewController.swift

+            }
+
+            viewController.setRightBarItemIdle()
+            viewController.dismiss(animated: true)


Note: this is strongly retaining the viewController param

rnystrom · 2018-10-27T19:37:33Z

Classes/Issues/IssuesViewController.swift


+    func sendEditTitleRequest(newTitle: String, viewController: EditIssueTitleViewController) {


Why not just put all the sending stuff inside the edit VC? Can include the cache updating stuff since the issue VC listens for model changes and will reload. Would totally eliminate the need for this delegate API and keep things really separated.

rnystrom · 2018-10-27T19:38:45Z

Classes/Issues/IssuesViewController.swift

+                Squawk.show(error: error)
+            }
+
+            viewController.setRightBarItemIdle()


So the edit VC sets its right bar spinning, but the issue VC is required to set it back? Adds a lot of implicit logic. Another +1 for just putting all this logic inside the edit VC...

rnystrom · 2018-10-31T13:10:05Z

@BrianLitwin just lmk when this is ready for another look! I'm going to try to get the push notifications release out and then fast-follow w/ another release including this, that way this PR has time to bake but doesn't slow down the next update.

ijm8710 · 2018-11-15T15:33:39Z

Hey @BrianLitwin you still planning to jump back on this? No rush at all, just checking in since it seemed you were almost done with it. Obv I’m just excited for this awesome addition and let me know if I’m overstepping by f/u and I’ll make sure I refrain from doing so again in future...

rnystrom · 2018-12-29T20:57:08Z

@BrianLitwin lmk if you have plans to revisit this. Def a high-impact feature! Leaving it open for now.

BrianLitwin · 2018-12-29T21:09:44Z

@rnystrom Thx for leaving it open- I def plan to re-submit this in the near future

_{Sent with GitHawk}

BrianLitwin · 2019-01-06T14:01:19Z

@rnystrom with the new Bookmark storage model, there's no reason to worry about updating the Bookmark Store anymore if you edited an Issue's title? Because we are not saving the bookmark title anymore, but saving the graphQL id and then fetching the title + other info from graphQL?

This has three pieces of key functionality: * sends a v3 network request to update the issue title * updates the IssueTitleSectionController (the title the user sees when they open an Issue) * updates the Issue Timeline These can be tested/verified manually. I also added a unit test to verify that the User's interaction with the EditIssueTitleViewController works as expected, eg a network request won't be sent unless a user edits the issue title and calls onSave(). This is an update of GitHawkApp#2348 - in this version of the PR, I'm using the pipeline IssueManagingContextController uses w/ it's other view controllers, ie updating things optimistically and triggered by the contextMenuWillDismiss() method. I didn't add any funcionality to update the Bookmarks api with the new title b/c my understanding is that those are not stored anymore, are requested from the github api. I updated some string constants (requested in original pr) and I removed the width parameter from a helper function [titleStringSizing] because it was convenient and I couldn't find anywhere that parameter was used Something to note is that issue titles that have already been requested and displayed won't be updated. Eg if you go from Bookmarks to an issue and edit the title and go back to bookmarks, the issue title on the bookmark won't be updated. [titleStringSizing]: https://github.com/GitHawkApp/GitHawk/blob/45e9cf6428d5edfe00a73eb18450c50c03871510/Classes/Issues/IssueViewModels.swift#L13

This has three pieces of key functionality: * sends a v3 network request to update the issue title * updates the IssueTitleSectionController (the title the user sees when they open an Issue) * updates the Issue Timeline These can be tested/verified manually. I also added a unit test to verify that the User's interaction with the EditIssueTitleViewController works as expected, eg a network request won't be sent unless a user edits the issue title and calls onSave(). This is an update of GitHawkApp#2348 - in this version of the PR, I'm using the IssueManagingContextController pipineline that it uses w/ it's other view controllers, ie updating optimistically and triggered by the contextMenuWillDismiss() method. EditIssueTitle's behavior is intended to be very similar to Labels, People, and Milestones. I didn't add any funcionality to update the Bookmarks api with the new title b/c my understanding is that those are not stored anymore but are requested from the github api. I updated some string constants (requested in original pr) and I removed the width parameter from a helper function [titleStringSizing] because it was convenient and I couldn't find anywhere that parameter was used [titleStringSizing]: https://github.com/GitHawkApp/GitHawk/blob/45e9cf6428d5edfe00a73eb18450c50c03871510/Classes/Issues/IssueViewModels.swift#L13

This has three pieces of key functionality: * sends a v3 network request to update the issue title * updates the IssueTitleSectionController (the title the user sees when they open an Issue) * updates the Issue Timeline These can be tested/verified manually. I also added a unit test to verify that the User's interaction with the EditIssueTitleViewController works as expected, eg a network request won't be sent unless a user edits the issue title and calls onSave(). This is an update of GitHawkApp#2348 - in this version of the PR, EditIssueTitle's behavior is intended to be very similar to Labels, People, and Milestones. Ie it updates the UI optimistically and these updates are triggered by ContextMenuWillDismiss(). I didn't add any funcionality to update Bookmarks after an issue title is edited because my understanding is that the bookmark titles are not stored anymore but are retrieved from the github api. I updated some string constants (requested in original pr) and I removed the width parameter from a helper function [titleStringSizing] because it was convenient and I couldn't find anywhere that parameter was used [titleStringSizing]: https://github.com/GitHawkApp/GitHawk/blob/45e9cf6428d5edfe00a73eb18450c50c03871510/Classes/Issues/IssueViewModels.swift#L13

Sherlouk approved these changes Oct 23, 2018

View reviewed changes

Sherlouk mentioned this pull request Oct 23, 2018

Handle PR "Suggestions" #2349

Open

BrianLitwin force-pushed the EditIssueTitle branch 2 times, most recently from 760aafc to 9ad2ed5 Compare October 23, 2018 23:44

BrianLitwin commented Oct 24, 2018

View reviewed changes

BrianLitwin force-pushed the EditIssueTitle branch 2 times, most recently from aa85c53 to 3b31118 Compare October 27, 2018 18:08

Adds edit issue title

7c0360a

Use localized string for title

BrianLitwin force-pushed the EditIssueTitle branch from 3b31118 to 7c0360a Compare October 27, 2018 18:18

rnystrom suggested changes Oct 27, 2018

View reviewed changes

BrianLitwin changed the title ~~Adds edit issue title~~ [WIP] Adds edit issue title Oct 30, 2018

BrianLitwin force-pushed the EditIssueTitle branch 3 times, most recently from 530084b to 25b9037 Compare October 30, 2018 18:35

move fetch to client

a33747b

BrianLitwin force-pushed the EditIssueTitle branch from 25b9037 to a33747b Compare October 30, 2018 22:01

updates w/ master

b2a4cd7

BrianLitwin force-pushed the EditIssueTitle branch 2 times, most recently from 1e9d8f9 to b2a4cd7 Compare November 6, 2018 20:32

update right bar btn

331df69

BrianLitwin force-pushed the EditIssueTitle branch from bff9c30 to 331df69 Compare November 6, 2018 20:36

Huddie added the 🚧 wip Work in progress label Dec 2, 2018

BrianLitwin mentioned this pull request Jan 20, 2019

Edit issue title #2618

Open

BrianLitwin closed this Jan 20, 2019

ijm8710 mentioned this pull request Jul 23, 2019

General question of the github app roadmap! #2806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Adds edit issue title #2348

[WIP] Adds edit issue title #2348

BrianLitwin commented Oct 23, 2018 •

edited

Loading

Sherlouk left a comment

Sherlouk Oct 23, 2018

BrianLitwin commented Oct 23, 2018

BrianLitwin Oct 24, 2018

BrianLitwin commented Oct 27, 2018 •

edited

Loading

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom Oct 27, 2018

rnystrom commented Oct 31, 2018 •

edited

Loading

ijm8710 commented Nov 15, 2018

rnystrom commented Dec 29, 2018

BrianLitwin commented Dec 29, 2018

BrianLitwin commented Jan 6, 2019


		func sendEditTitleRequest(newTitle: String, viewController: EditIssueTitleViewController) {

[WIP] Adds edit issue title #2348

[WIP] Adds edit issue title #2348

Conversation

BrianLitwin commented Oct 23, 2018 • edited Loading

Update:

Sherlouk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianLitwin commented Oct 23, 2018

Choose a reason for hiding this comment

BrianLitwin commented Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnystrom commented Oct 31, 2018 • edited Loading

ijm8710 commented Nov 15, 2018

rnystrom commented Dec 29, 2018

BrianLitwin commented Dec 29, 2018

BrianLitwin commented Jan 6, 2019

BrianLitwin commented Oct 23, 2018 •

edited

Loading

BrianLitwin commented Oct 27, 2018 •

edited

Loading

rnystrom commented Oct 31, 2018 •

edited

Loading