Allow adding and viewing reviews to PRs

[ericadamski]

This app is great! Only thing missing for me is the ability to leave comments on specific lines of a PR and to allow adding a review (approved/request changes).

I would be willing to do the work for this assuming this is useful functionality that others would like.

[housseindjirdeh]

Thank you so much @ericadamski

Yes this is something we really need. Currently we don't show any pull request comments whatsoever (PR open #6) and it's kinda misleading. Moreover being able to add reviews to PRs would be huge :)

We would love your help on this, I'll close the previous ticket since they're somewhat related.

[housseindjirdeh]

So here's what I'm thinking, maybe we can start with showing all the PR review comments first before we tackle allowing the user to add review comments? First thing that comes to mind is in the PullDiffScreen that shows the code changes for a pull request, show the actual comments inline similar to GitHub currently:

We can also include those comments with an indicator (or slightly different) style in the issue conversation as well?

^ I feel like that might not be the easiest/best way to do things. Maybe have a separate screen to show different pull request reviews in addition to having it in the diff screen?

If we do decide to include it in the issue conversation screen or have it in a separate screen, I still think it makes sense to first focus on including it only in the Diff screen and including that in the app before rolling out any further additions.

Open to all kinds of suggestions as always. 💬

CC @machour @andrewda @tautf @chinesedfan @lex111

[ericadamski]

This all sounds great! My only suggestion would be that if we were to add the code diff in the comments that we maybe just put a link to the location of the comment in the actual diff screen rather than crowd the conversation log with code snippets.

[ericadamski]

@housseindjirdeh just going through and adding PR review comments on PR's and noticing that I am attempting to shove my code in to the current state of the app.

I am wondering how you all feel about the format of the store and if you are ok with me re-writing the state a little. Reason being since the issue key in the store gets reused when switching between two different issue number (this includes PRs as well since github treats them the same) some data does not get reset.

I would like to propose a change of the issue key to be a sub-store type structure of issues and then use combineReducers to contain similar logical structures within the same sub-store and use reselect to cherry-pick and memoize the store.

My suggestion is to key each entry by the ID given from github, for example:

{ // Store Top Level
     issues: { // Issues "Sub-Store"
          1: {
               comments: 1,
               /* ... */
           },
          [issue.id]: { /* raw issue object */ }
     }
}

The caveat to this is the app navigation will now need the issue id to be passed around, which for the benefit of the store organization and memoization this should have little impact.

I do not mind including this refactoring within this task here.

Open to all kinds of suggestions as always. 💬

CC @machour @andrewda @tautf @chinesedfan @lex111

[machour]

@ericadamski I've been working on a complete rewrite of the state that will solve the issue you're currently facing. (ETA to master in early November)

I'm personally not sure it's worth worrying about this for now, but let's see what the team have to say :)

[ericadamski]

@machour the problem is that if I add the functionality to do review comments I will be modifying the store in an odd way, it would be a more sustainable approach to reorganize the parts of the store that I am touching. Is it possible to get a sneak peak at how you are rebuilding so I can mimic that and still complete the review comment functionality?

[machour]

@ericadamski here's the initial PR I opened: #457. You can go through its description to grasp the introduced changes, and if needed hit me on gitter for more information.

Please don't hesitate to implement what you envisioned for now, I'd hate for the refactoring to be a blocker.

[housseindjirdeh]

Thank you @ericadamski, and yep I already agree with you. We definitely need to have a more normalized data structure.

As @machour mentioned, he's currently modifying things to match that format in the store piece by piece. I think the best approach here would be for you to go right ahead and modify the store in a more sustainable manner (I agree it's kinda funky now!). I hope it doesn't cause too much conflict burden on your part however @machour :O

@ericadamski don't hesitate to hop on the gitter chat if you need us to explain how @machour's currently envisioning his store changes or if you'd like to pick our brains about anything.

[andrewda]

This should definitely be included in v1.5.0 if we can get it working well. It's currently the only thing preventing me from using only GitPoint instead of switching between it and a browser. I can probably start doing some basic design on this if nobody else is currently working on it?