slapperwan / gh4a

Github client for Android
Apache License 2.0
1.69k stars 221 forks source link

Add support for Pull Request Reviews and Review Requests #391

Open m-deck opened 7 years ago

m-deck commented 7 years ago

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

sbrl commented 7 years ago

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

Tunous commented 7 years ago

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

Tunous commented 7 years ago

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.

sbrl commented 7 years ago

@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.

maniac103 commented 7 years ago

@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).

Tunous commented 7 years ago

peek 2017-06-10 13-22

Something like this?

Tunous commented 7 years ago

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 commented 7 years ago

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)

maniac103 commented 7 years ago

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

Tunous commented 7 years ago

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

Tunous commented 7 years ago

spectacle m15193

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

maniac103 commented 7 years ago

630 is merged now -> closing

Tunous commented 7 years ago

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.

maniac103 commented 7 years ago

OK, nevermind then :-)

smichel17 commented 7 years ago

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

Tunous commented 7 years ago

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 commented 7 years ago

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.

smichel17 commented 7 years ago

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

Tunous commented 6 years ago

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