Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add pr review icons (approved, requested changes, commented, dismissed) #1078

Merged
merged 17 commits into from May 6, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented May 2, 2024

Closes #682

For Pull Request notifications, when `detailed notifications setting is enabled, fetch PR reviews and find the most recent for the authenticated user

Will show an additional icon on the notification row, as pictured below.

Screenshot 2024-05-01 at 3 18 05 PM

Feedback Areas
I'd appreciate feedback on the following:

  • are there better positions for the icon?
  • mouse-over text (closely modeled off the GitHub UI wording)

Broader discussion
This has also got me thinking more generally if we could/should show PR Approval icons for all reviewers. If so, what would we show when there is multiple people with feedback?

@setchy setchy changed the title Feature/pr review indicator feat: add pr review icons (approved, requested changes, commented, dismissed) May 2, 2024
@setchy setchy marked this pull request as draft May 2, 2024 01:57
@setchy setchy marked this pull request as ready for review May 2, 2024 06:39
@setchy
Copy link
Member Author

setchy commented May 2, 2024

@bhingston-va - as the feature requestor - would really value your input on this 🙏

@afonsojramos
Copy link
Member

How does it look on hover? When there's more icons?

@bhingston-va
Copy link

Looks great to me! This is awesome. I'll be honest this was more of need when the notifications would duplicate/inconsistent state. This inconsistent state was fixed in v5.4.0 I believe. That said this would be really cool. Thanks for actioning on it so fast. 🎉

@bhingston-va
Copy link

bhingston-va commented May 2, 2024

How does it look on hover? When there's more icons?

Looking again. On hover, I see the other icons likely collide or get squished. Also be confusing with the new "approved" icon and the existing "Mark as done" icon on hover (as they are the same icon). I wonder if putting this new icon with the "latest update". With the idea it is just our review and could be later iterated on to support multiple reviewers by having the picture of the reviewer with the icon of the review state.

before after
Screenshot 2024-05-02 at 8 52 01 AM Screenshot 2024-05-02 at 8 52 01 AM

@setchy
Copy link
Member Author

setchy commented May 2, 2024

Happy to try adding the icons to the footer as you've mocked up and see how that goes.

Perhaps to generalize the solution, we could

  • Parse the reviews array
  • Find the most recent review state for each reviewer
  • Show up to 3 icons per notification footer (approved, changes requested, commented/dismissed)
  • Dynamically generate the mouse-over text to summarize which users have left which reviews

@bhingston-va
Copy link

bhingston-va commented May 2, 2024

Show up to 3 icons per notification footer (approved, changes requested, commented/dismissed)

Minor detail, consider always including "my review" if it exists. Up to 3 icons I like but either 3 icons of others or my review and 2 others when there are 3 or more reviews. Ah, I understand your suggestion for multi reviews. Show all or at least up to 3 review states (I see you are adding dismissed so maybe 4 states/icons) and on hover you show who all did that review state. I like it a lot.

I do like the original at a glance to see my review. That said showing all review states and I just need to hover to see who did them is a general solution with more value (at the cost of a hover to see my review if any).

@setchy
Copy link
Member Author

setchy commented May 3, 2024

Following the above discussion, I've pushed updates that now

  • add the icons to the notification row footer
  • fetch the latest review for each reviewer
  • generate appropriate descriptions
Screenshot 2024-05-03 at 6 27 57 AM Screenshot 2024-05-03 at 6 28 06 AM

@setchy
Copy link
Member Author

setchy commented May 3, 2024

Here is an example of multiple review states using stubbed data

Screenshot 2024-05-03 at 8 36 02 AM

For now I made the decision to show Commented as YELLOW and Dismissed as GRAY so that they've visually different (in the GitHub UI they're combined into a single GRAY icon)

src/utils/subject.ts Outdated Show resolved Hide resolved
src/utils/subject.ts Outdated Show resolved Hide resolved
Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, looking good!

@setchy
Copy link
Member Author

setchy commented May 6, 2024

@bhingston-va - just wanted to check that this impl covers the enhancement you were hoping for?

@setchy setchy merged commit 454c7da into main May 6, 2024
7 checks passed
@setchy setchy deleted the feature/pr-review-indicator branch May 6, 2024 15:41
@setchy setchy added the enhancement New feature or enhancement to existing functionality label May 6, 2024
@setchy setchy added this to the Release 5.5.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
3 participants