gh4a icon indicating copy to clipboard operation
gh4a copied to clipboard

Add support for Pull Request Reviews and Review Requests

Open m-deck opened this issue 8 years ago • 19 comments

https://developer.github.com/v3/pulls/reviews/

  • [x] Existing reviews
    • [x] Loading
    • [x] Replying
    • [ ] Dismissing
  • [ ] Creating new reviews without comments
  • [ ] Pending reviews
    • [x] Loading
    • [ ] Creating
    • [ ] Submitting
    • [ ] Adding comments
  • [ ] Review requests
    • [ ] Timeline events
    • [ ] Adding or removing
    • [ ] Displaying

m-deck avatar Feb 01 '17 16:02 m-deck

This would be really useful as a maintainer of a project.

sbrl avatar Apr 20 '17 20:04 sbrl

Out of preview https://developer.github.com/changes/2017-05-09-end-black-cat-preview/

Tunous avatar May 09 '17 16:05 Tunous

spectacle x29041

Some in progress work. What you can see here is review groups with matching diffs displayed similar to how GitHub does it. (These diffs would display few lines of code) It's really messy right now but what do you think about this?

In finished version, I think it would be nice to group review blocks into cards to make it easy to see where they start and end. Similar how it works on GitHub mobile web version. And perhaps could add some way to collapse/expand them.

Tunous avatar Jun 07 '17 19:06 Tunous

@Tunous That looks great! And displaying them as cards is a fantastic idea. It doesn't have to display things identically to the web version, because sometimes other display methods make more sense.

sbrl avatar Jun 07 '17 20:06 sbrl

@Tunous I think I'd prefer displaying a short summary ('Tunous reviewed on xxx\nCode comments in file yyy, zzz\nShow details') and show the details in a separate event list fragment in a new activity. Otherwise I think the list will become either confusing/hard to parse (your prototype) and/or visually jarring (nested list).

maniac103 avatar Jun 08 '17 10:06 maniac103

peek 2017-06-10 13-22

Something like this?

Tunous avatar Jun 10 '17 11:06 Tunous

spectacle m11874

With reply buttons of course. Commenting here is less linear so it should work differently: Bottom sheet would be disabled by default here and only activate once you press one of the reply. And the reply button would be highlighted so you see to which group you are replying.

The diffs would display few lines of code and could be also clickable either as a whole or with a smaller button. Clicking it would open corresponding file and scroll to the diff.

Tunous avatar Jun 10 '17 11:06 Tunous

peek 2017-06-10 13-32

Example with real conversations from this pr. You can test it yourself in reviews branch: https://github.com/Tunous/gh4a/tree/reviews. (Code is really messy right now, I'll clean up once everything works)

Tunous avatar Jun 10 '17 11:06 Tunous

Looks good. I assume the big green 'diff' block is a placeholder?

maniac103 avatar Jun 10 '17 11:06 maniac103

Yes, I plan on displaying it like GitHub does (line numbers, text, red/green bgs)

Tunous avatar Jun 10 '17 11:06 Tunous

spectacle m15193

Like this but I need to figure out how to style it with text spans.

Tunous avatar Jun 10 '17 12:06 Tunous

#630 is merged now -> closing

maniac103 avatar Sep 08 '17 06:09 maniac103

No, not yet. There is also review creation part missing that I'm still working on. Right now we only have a way to view reviews which isn't what a full support should be.

Tunous avatar Sep 08 '17 08:09 Tunous

OK, nevermind then :-)

maniac103 avatar Sep 08 '17 08:09 maniac103

Would it make sense to pin the reply bar to the bottom in the review view, instead of an action at the bottom?

smichel17 avatar Sep 08 '17 12:09 smichel17

No, there can be multiple comment threads in a single review and each of them has its own reply button.

EDIT: To clarify. If there would be a bar then it would be really easy for the user to forget to which thread they are replying and accidentally send an incorrect text. By making it a separate screen it's clear for the user to see to what they are replying.

Tunous avatar Sep 08 '17 12:09 Tunous

Actually it would be possible if we did it like in my other idea. (I don't remember where I wrote it)

Reply button would work as a focus point. If you click it, it gets highlighted to tell you that you are replying to its comments thread. At any time you could click on other reply buttons to change the focus. (accidently clicked wrong one?)

That way bottom sheet would be always visible and you would have ability to scroll through all the comments, quote them, copy text, etc.

I started work on this approach before but I don't remember why I changed my mind.... Probably because comment bottom sheet wasn't merged yet and I didn't want to wait for it.

Tunous avatar Sep 08 '17 13:09 Tunous

The way I was thinking about it when I made the suggestion was that:

  • On the PR conversation tab, show "Q reviewed this".
  • Tapping the review expands it to shows the comment threads (like the github web interface), rather than opening a new screen.
  • Each comment thread is a card that shows the file (title), a very abbreviated diff, the top comment, and a little bit of additional info ("X more comments"). Again, like the github web interface, except that web shows all the comments and we're just showing the top one.
  • Tapping the card opens up a new screen that shows more of the diff, all of the comments, and has the reply box pinned to the bottom

smichel17 avatar Sep 08 '17 14:09 smichel17

I've added checklist to the issue body to list exactly what is done and what else needs to be done.

Tunous avatar Nov 05 '17 11:11 Tunous