codeapprove / feedback

Feedback for CodeApprove
0 stars 0 forks source link

Make version-to-version diffs easier to use #70

Closed mtlynch closed 1 year ago

mtlynch commented 2 years ago

80% of the time, the "Show changes since my last review" is all I need. Another 10% of the time, "Show all changes" covers my needs. But there's a last 10% where I need to do fine-grained diffs between commits and CodeApprove is a little hard to use for that.

Comparing other tools

Reviewable

I think Reviewable does this the best. The visual display makes it obvious how the ordering works, and you can do it in a single click-and-drag mouse gesture. By comparison, Gerrit and CodeApprove are each 4 clicks.

https://user-images.githubusercontent.com/7783288/196443885-2f8220cf-e9ca-41d1-8345-d559d6f914fa.mp4

Gerrit

Gerrit is pretty similar to CodeApprove, though the arrow gives a better visual hint at the relationship between the dropdowns. They also sort the dropdowns consistently and disable invalid comparisons:

https://user-images.githubusercontent.com/7783288/196444462-f4fa70fc-4f67-4df1-a765-6a037895f9e4.mp4

samatcodeapprove commented 2 years ago

@mtlynch thank you for the super detailed feedback! I'd like to find something in between what I have now and the file matrix (which I find overwhelming) for CodeApprove.

samatcodeapprove commented 1 year ago

See https://github.com/codeapprove/feedback/issues/78, the UI should be a lot clearer now.

mtlynch commented 1 year ago

Cool, this is a good improvement!

My remaining nitpick is that you can still compare a commit to itself and generate an empty diff, which seems like something no user would ever actually want.

When base is at commit N, CodeApprove grays out every commit < N, but I think the more intuitive behavior is to gray out every commit <= N. This would also mean that master is always grayed out on the "head" dropdown.

Similarly, I'd prefer if the last commit would always be grayed out in the "base" dropdown because that always just generates an empty diff.

Also, not sure if it's related to these changes, but the list of commits now has a layout issue where the commit hashes are not vertically aligned and the dropdown scrolls both horizontally and vertically (example):

image

mtlynch commented 1 year ago

@samatcodeapprove - Should I open a separate issue for the remaining items? I just want to verify you've ACKed the follow up, even if it's to wontfix.

samatcodeapprove commented 1 year ago

@mtlynch thanks for the bump! Reopened as partially-complete to track the rest

samatcodeapprove commented 1 year ago

Deploying a fix for the remaining items now!

mtlynch commented 1 year ago

Cool, thanks! Fix verified.