Skip to content

Show review decision and reviewers in PR card#449

Merged
NorbertLoh merged 43 commits intoCATcher-org:mainfrom
wx-03:reviewers
Apr 7, 2025
Merged

Show review decision and reviewers in PR card#449
NorbertLoh merged 43 commits intoCATcher-org:mainfrom
wx-03:reviewers

Conversation

@wx-03
Copy link
Copy Markdown
Contributor

@wx-03 wx-03 commented Mar 23, 2025

Summary:

Fixes #408
Fixes #443

Type of change:

  • ✨ New Feature/ Enhancement

Changes Made:

  • Added a logo in the PR card to show the review decision (changes requested / review required / approved)
  • GitHub avatars of reviewers are shown in the PR card, with tooltips that show the username and review state (changes requested / approved / commented / dismissed / pending)
  • Added an icon to indicate when PRs have been merged without any review
  • Tests have not been added in this PR and should be done under the PR for issue Create unit tests for issue model #453.

Screenshots:

latest

When the icons and avatars are unable to fit into the same line as the labels and milestone, they will be wrapped to the next line:

image

Proposed Commit Message:

Show review decision and reviewers in PR card

Review decisions and reviewers are not shown in PR card.

Showing review decions and reviewers will allow users to see what needs
to be done and which PRs require reviews at a glance, and showing 
reviewers allows tutors to gauge the level of PR reviews in the team.
Displaying an icon for PRs that have been merged without reviews can 
help tutors to easily identify PRs that violate best practices for 
software engineering.

Let's ,
* display review decions in the PR card,
* display the GitHub avatars of PR reviewers in the PR card,
* use tooltips to show information about reviews, and
* display an icon for PRs that have been merged without any review.

Checklist:

  • I have tested my changes thoroughly.
  • I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
  • I have added or modified code comments to improve code readability where necessary.
  • I have updated the project's documentation as necessary.

@wx-03 wx-03 marked this pull request as draft March 23, 2025 06:37
@damithc
Copy link
Copy Markdown
Contributor

damithc commented Mar 23, 2025

@wx-03 shall we show all these stuff in the same line (but overflow to next line if cannot fit in one line), to optimise vertical space usage?
image

@wx-03
Copy link
Copy Markdown
Contributor Author

wx-03 commented Mar 26, 2025

@wx-03 shall we show all these stuff in the same line (but overflow to next line if cannot fit in one line), to optimise vertical space usage? image

I have updated it to show the review decision icon on the right side of the reviewers, and it will now be on the same line as the labels and milestone, unless there is not enough space.

@wx-03 wx-03 marked this pull request as ready for review March 26, 2025 05:57
Copy link
Copy Markdown
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

The functionality looks great!

For minor improvements, some small refactoring can be done, otherwise its lgtm


I also noticed some unusual errors that seem to be coming from GitHub's side:
image

It’s unclear if this requires additional logic to address since its a respond from the graphql servers (I think), so we can leave this as a potential future issue for now.

@wx-03 wx-03 marked this pull request as draft March 27, 2025 11:15
@wx-03 wx-03 marked this pull request as ready for review March 28, 2025 10:28
@wx-03
Copy link
Copy Markdown
Contributor Author

wx-03 commented Mar 28, 2025

The functionality looks great!

For minor improvements, some small refactoring can be done, otherwise its lgtm

I also noticed some unusual errors that seem to be coming from GitHub's side: image

It’s unclear if this requires additional logic to address since its a respond from the graphql servers (I think), so we can leave this as a potential future issue for now.

@Eclipse-Dominator Thanks for the review, I have made the necessary changes.

@wx-03 wx-03 requested a review from Eclipse-Dominator March 28, 2025 10:29
Copy link
Copy Markdown
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

LGTM for me.
Let's see what others think about the design changes.

@NorbertLoh NorbertLoh merged commit bf7fa05 into CATcher-org:main Apr 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show reviewers in PR card Display information regarding PR reviews and merging statuses

4 participants