ui: refactor display of review threads on pr view (#8138)

* refactored to untangle comment headers of different styles of comments from each other
* fixed misalignments
* improved consistency
* fixed missing avatar rounding
* made reactions aligned with content
* fixed text in the "Outdated" label overflowing into the toggle icon

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8138
Reviewed-by: Beowulf <beowulf@beocode.eu>

templates/repo/diff/comment_form.tmpl

@@ -1,5 +1,5 @@
 {{if and $.root.SignedUserID (not $.Repository.IsArchived)}}
-	<form class="ui form {{if $.hidden}}tw-hidden comment-form{{end}}" action="{{$.root.Issue.Link}}/files/reviews/comments" method="post">
+	<form class="ui form {{if $.hidden}}tw-hidden comment-form tw-mt-2{{end}}" action="{{$.root.Issue.Link}}/files/reviews/comments" method="post">
 	{{$.root.CsrfTokenHtml}}
 		<input type="hidden" name="origin" value="{{if $.root.PageIsPullFiles}}diff{{else}}timeline{{end}}">
 		<input type="hidden" name="latest_commit_id" value="{{$.root.AfterCommitID}}">
@@ -25,22 +25,22 @@
 			</div>
 		{{end}}
 
-		<div class="field footer tw-mx-2">
+		<div class="field footer">
 			<span class="markup-info">{{svg "octicon-markdown"}} {{ctx.Locale.Tr "repo.diff.comment.markdown_info"}}</span>
-			<div class="tw-text-right">
+			<div class="right button-sequence">
 				{{if $.reply}}
-					<button class="ui submit primary tiny button btn-reply" type="submit">{{ctx.Locale.Tr "repo.diff.comment.reply"}}</button>
+					<button class="ui submit primary tiny button" type="submit">{{ctx.Locale.Tr "repo.diff.comment.reply"}}</button>
 					<input type="hidden" name="reply" value="{{$.reply}}">
 				{{else}}
 					{{if $.root.CurrentReview}}
-						<button name="pending_review" type="submit" class="ui submit primary tiny button btn-add-comment">{{ctx.Locale.Tr "repo.diff.comment.add_review_comment"}}</button>
+						<button name="pending_review" type="submit" class="ui submit primary tiny button">{{ctx.Locale.Tr "repo.diff.comment.add_review_comment"}}</button>
 					{{else}}
-						<button name="pending_review" type="submit" class="ui submit primary tiny button btn-start-review">{{ctx.Locale.Tr "repo.diff.comment.start_review"}}</button>
+						<button name="pending_review" type="submit" class="ui submit primary tiny button">{{ctx.Locale.Tr "repo.diff.comment.start_review"}}</button>
 						<button name="single_review" value="true" type="submit" class="ui submit tiny basic button btn-add-single">{{ctx.Locale.Tr "repo.diff.comment.add_single_comment"}}</button>
 					{{end}}
 				{{end}}
 				{{if or (not $.HasComments) $.hidden}}
-					<button type="button" class="ui submit tiny basic button btn-cancel cancel-code-comment">{{ctx.Locale.Tr "cancel"}}</button>
+					<button type="button" class="ui submit tiny basic button cancel-code-comment">{{ctx.Locale.Tr "cancel"}}</button>
 				{{end}}
 			</div>
 		</div>

templates/repo/issue/view_content/conversation.tmpl

@@ -3,11 +3,11 @@
 {{$resolveDoer := (index .comments 0).ResolveDoer}}
 {{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}}
 <div class="ui segments conversation-holder">
-	<div class="ui segment collapsible-comment-box tw-py-2 tw-flex tw-items-center tw-justify-between">
+	<div class="ui segment collapsible-comment-box tw-py-2 tw-gap-2 tw-flex tw-items-center tw-justify-between">
-		<div class="tw-flex tw-items-center">
+		<div class="tw-flex tw-items-center tw-gap-2">
-			<a href="{{(index .comments 0).CodeCommentLink ctx}}" class="file-comment tw-ml-2 tw-break-anywhere">{{(index .comments 0).TreePath}}</a>
+			<a href="{{(index .comments 0).CodeCommentLink ctx}}" class="file-comment tw-break-anywhere">{{(index .comments 0).TreePath}}</a>
 			{{if $invalid}}
-				<span class="ui label tw-ml-2" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.review.outdated_description"}}">
+				<span class="ui label" data-tooltip-content="{{ctx.Locale.Tr "repo.issues.review.outdated_description"}}">
 					{{ctx.Locale.Tr "repo.issues.review.outdated"}}
 				</span>
 			{{end}}
@@ -49,19 +49,18 @@
 		</div>
 	{{end}}
 	<div id="code-comments-{{(index .comments 0).ID}}" class="comment-code-cloud ui segment{{if $resolved}} tw-hidden{{end}}">
-		<div class="ui comments tw-mb-0">
 		{{range .comments}}
 			{{$createdSubStr:= DateUtils.TimeSince .CreatedUnix}}
-				<div class="comment code-comment tw-pb-4" id="{{.HashTag}}">
+			<div class="comment code-comment" id="{{.HashTag}}">
 				<div class="content">
-						<div class="header comment-header">
+					<div class="header">
-							<div class="comment-header-left tw-flex tw-items-center">
+						<div class="comment-header-left tw-flex">
 							{{if not .OriginalAuthor}}
 								<a class="avatar">
 									{{ctx.AvatarUtils.Avatar .Poster 20}}
 								</a>
 							{{end}}
-								<span class="text grey muted-links">
+							<span class="text grey muted-links tw-self-center">
 								{{if .OriginalAuthor}}
 									<span class="text black">
 										{{svg (MigrationIcon $.Repository.GetOriginalURLHostname)}}
@@ -105,8 +104,7 @@
 				</div>
 			</div>
 		{{end}}
-		</div>
+		<div class="tw-flex tw-items-center tw-flex-wrap tw-mt-2">
-		<div class="tw-flex tw-items-center tw-flex-wrap tw-mt-2 tw-mb-1 tw-mx-2">
 			<div class="tw-flex-1">
 				{{if $resolved}}
 					<div class="ui grey text">

tests/e2e/pr-review.test.e2e.ts

@@ -47,7 +47,7 @@ test('PR: Create review from commit', async ({page}) => {
   if (await start_button.isVisible({timeout: 100})) {
     await start_button.click();
   } else {
-    await page.locator('.comment-code-cloud form button.btn-add-comment').click();
+    await page.locator('.comment-code-cloud form button[name="pending_review"]').click();
   }
 
   await expect(page.locator('.comment-list .comment-container')).toBeVisible();

tests/integration/comment_roles_test.go

@@ -1,5 +1,5 @@
-// Copyright 2024 The Forgejo Authors. All rights reserved.
+// Copyright 2024-2025 The Forgejo Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: GPL-3.0-or-later
 
 package integration
 
@@ -10,6 +10,8 @@ import (
 	"strings"
 	"testing"
 
+	"forgejo.org/modules/translation"
+
 	"github.com/PuerkitoBio/goquery"
 	"github.com/stretchr/testify/assert"
 )
@@ -19,11 +21,12 @@ func TestCommentRoles(t *testing.T) {
 	user := "user2"
 	repo := "repo1"
 
-	ownerTooltip := "This user is the owner of this repository."
+	locale := translation.NewLocale("en-US")
-	authorTooltipPR := "This user is the author of this pull request."
+	authorTooltipPR := locale.TrString("repo.issues.author.tooltip.pr")
-	authorTooltipIssue := "This user is the author of this issue."
+	authorTooltipIssue := locale.TrString("repo.issues.author.tooltip.issue")
-	contributorTooltip := "This user has previously committed in this repository."
+	ownerTooltip := locale.TrString("repo.issues.role.owner_helper")
-	newContributorTooltip := "This is the first contribution of this user to the repository."
+	contributorTooltip := locale.TrString("repo.issues.role.contributor_helper")
+	newContributorTooltip := locale.TrString("repo.issues.role.first_time_contributor_helper")
 
 	// Test pulls
 	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
@@ -32,7 +35,7 @@ func TestCommentRoles(t *testing.T) {
 		sessionUser11 := loginUser(t, "user11")
 
 		// Open a new PR as user2
-		testEditFileToNewBranch(t, sessionUser2, user, repo, "master", "comment-labels", "README.md", "test of comment labels\naline")
+		testEditFileToNewBranch(t, sessionUser2, user, repo, "master", "comment-labels", "README.md", "test of comment labels\naline") // Owner
 		sessionUser2.MakeRequest(t, NewRequestWithValues(t, "POST", path.Join(user, repo, "compare", "master...comment-labels"),
 			map[string]string{
 				"_csrf": GetCSRF(t, sessionUser2, path.Join(user, repo, "compare", "master...comment-labels")),
@@ -52,7 +55,7 @@ func TestCommentRoles(t *testing.T) {
 		page := NewHTMLParser(t, response.Body)
 		replyID, _ := page.Find(".comment-form input[name='reply']").Attr("value")
 
-		testEasyLeavePRReviewComment(t, sessionUser2, user, repo, testID, "README.md", "1", "Another review comment from user2 on this line", replyID)
+		testEasyLeavePRReviewComment(t, sessionUser1, user, repo, testID, "README.md", "1", "Reply comment from a contributor", replyID)
 		testEasyLeavePRComment(t, sessionUser2, user, repo, testID, "New comment from user2 on this PR")   // Author, Owner
 		testEasyLeavePRComment(t, sessionUser1, user, repo, testID, "New comment from user1 on this PR")   // Contributor
 		testEasyLeavePRComment(t, sessionUser11, user, repo, testID, "New comment from user11 on this PR") // First-time contributor
@@ -60,39 +63,47 @@ func TestCommentRoles(t *testing.T) {
 		// Fetch the PR page
 		response = sessionUser2.MakeRequest(t, NewRequest(t, "GET", path.Join(user, repo, "pulls", testID)), http.StatusOK)
 		page = NewHTMLParser(t, response.Body)
-		commentHeads := page.Find(".timeline .comment .comment-header .comment-header-right")
-		assert.Equal(t, 6, commentHeads.Length())
 
-		// Test the first comment and it's label "Owner"
+		reviewHeads := page.Find(".timeline .code-comment .header .comment-header-right")
-		labels := commentHeads.Eq(0).Find(".role-label")
+		assert.Equal(t, 2, reviewHeads.Length())
+		commentHeads := page.Find(".timeline .comment .comment-header .comment-header-right")
+		assert.Equal(t, 4, commentHeads.Length())
+
+		// === Review comments ===
+
+		// Test the first review comment labels
+		labels := reviewHeads.Eq(0).Find(".role-label")
+		assert.Equal(t, 2, labels.Length())
+		testIssueCommentUserLabel(t, labels.Eq(0), "Author", authorTooltipPR)
+		testIssueCommentUserLabel(t, labels.Eq(1), "Owner", ownerTooltip)
+
+		// Test the second review comment labels
+		labels = reviewHeads.Eq(1).Find(".role-label")
+		assert.Equal(t, 1, labels.Length())
+		testIssueCommentUserLabel(t, labels.Eq(0), "Contributor", contributorTooltip)
+
+		//== Top comment ==
+
+		// Top comment (PR description) never shows `Author` label because it is implied
+		labels = commentHeads.Eq(0).Find(".role-label")
 		assert.Equal(t, 1, labels.Length())
 		testIssueCommentUserLabel(t, labels.Eq(0), "Owner", ownerTooltip)
 
-		// Test the second (review) comment and it's labels "Author" and "Owner"
+		// === Regular comments ===
+
+		// Test the first regular comment labels
 		labels = commentHeads.Eq(1).Find(".role-label")
 		assert.Equal(t, 2, labels.Length())
 		testIssueCommentUserLabel(t, labels.Eq(0), "Author", authorTooltipPR)
 		testIssueCommentUserLabel(t, labels.Eq(1), "Owner", ownerTooltip)
 
-		// Test the third (review) comment and it's labels "Author" and "Owner"
+		// Test the second regular comment labels
 		labels = commentHeads.Eq(2).Find(".role-label")
-		assert.Equal(t, 2, labels.Length())
-		testIssueCommentUserLabel(t, labels.Eq(0), "Author", authorTooltipPR)
-		testIssueCommentUserLabel(t, labels.Eq(1), "Owner", ownerTooltip)
-
-		// Test the fourth comment and it's labels "Author" and "Owner"
-		labels = commentHeads.Eq(3).Find(".role-label")
-		assert.Equal(t, 2, labels.Length())
-		testIssueCommentUserLabel(t, labels.Eq(0), "Author", authorTooltipPR)
-		testIssueCommentUserLabel(t, labels.Eq(1), "Owner", ownerTooltip)
-
-		// Test the fivth comment and it's label "Contributor"
-		labels = commentHeads.Eq(4).Find(".role-label")
 		assert.Equal(t, 1, labels.Length())
 		testIssueCommentUserLabel(t, labels.Eq(0), "Contributor", contributorTooltip)
 
-		// Test the sixth comment and it's label "First-time contributor"
+		// Test the third regular comment labels
-		labels = commentHeads.Eq(5).Find(".role-label")
+		labels = commentHeads.Eq(3).Find(".role-label")
 		assert.Equal(t, 1, labels.Length())
 		testIssueCommentUserLabel(t, labels.Eq(0), "First-time contributor", newContributorTooltip)
 	})

tests/integration/pull_review_test.go

@@ -194,7 +194,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
 		// even on template error, the page returns HTTP 200
 		// count the comments to ensure success.
 		doc = NewHTMLParser(t, resp.Body)
-		assert.Len(t, doc.Find(`.comments > .comment`).Nodes, 1)
+		assert.Len(t, doc.Find(`.comment-code-cloud > .comment`).Nodes, 1)
 	})
 
 	t.Run("outdated and newer review (line 2)", func(t *testing.T) {
@@ -311,7 +311,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
 		// even on template error, the page returns HTTP 200
 		// count the comments to ensure success.
 		doc = NewHTMLParser(t, resp.Body)
-		comments := doc.Find(`.comments > .comment`)
+		comments := doc.Find(`.comment-code-cloud > .comment`)
 		assert.Len(t, comments.Nodes, 1) // the outdated comment belongs to another review and should not be shown
 	})
 
@@ -343,7 +343,7 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
 		resp := session.MakeRequest(t, req, http.StatusOK)
 
 		doc := NewHTMLParser(t, resp.Body)
-		comments := doc.Find(`.comments > .comment`)
+		comments := doc.Find(`.comment-code-cloud > .comment`)
 		assert.Len(t, comments.Nodes, 3) // 1 comment on line 1 + 2 comments on line 3
 	})
 }

web_src/css/repo.css

@@ -984,23 +984,57 @@ pdf-object {
 }
 
 .repository.view.issue .comment-list .code-comment {
-  border: 1px solid transparent;
   margin: 0;
+  /* top: around 5px of whitespace are added by reaction octicon. (0.5rem=7px)-5px=2px */
+  padding: 2px 0.5rem 1.5rem;
 }
 
-.repository.view.issue .comment-list .code-comment .comment-header {
+@media (max-width: 768px) {
-  background: transparent;
+  .repository.view.issue .comment-list .code-comment {
-  border-bottom: 0 !important;
+    padding: 0.33rem 0.33rem 1.33rem;
-  padding: 0 !important;
+  }
 }
 
-.repository.view.issue .comment-list .code-comment .comment-header::after,
+.repository.view.issue .comment-list .code-comment:target {
-.repository.view.issue .comment-list .code-comment .comment-header::before {
+  outline: 1px solid var(--color-primary);
-  display: none;
+}
+
+.repository.view.issue .comment-list .code-comment .header {
+  display: flex;
+  margin-bottom: 0.25rem;
+  justify-content: space-between;
+  align-items: center;
+}
+
+.repository.view.issue .comment-list .code-comment .header .comment-header-left {
+  gap: 0.5rem;
+}
+
+.repository.view.issue .comment-list .code-comment .header .avatar {
+  margin: 0 !important;
 }
 
 .repository.view.issue .comment-list .code-comment .comment-content {
-  margin-left: 36px;
+  margin-left: calc(20px + 0.5rem);
+}
+
+.repository.view.issue .comment-list .code-comment .add-reaction {
+  padding: 0 0.5rem;
+}
+
+/* emulation of horizontal rule between comments originating from forgejo- themes, commit faab0c670e */
+
+.repository.view.issue .comment-list .code-comment + .code-comment {
+  margin-top: 1rem !important;
+  padding-top: 1rem !important;
+  border-top: 1px solid var(--color-secondary) !important;
+}
+
+@media (max-width: 768px) {
+  .repository.view.issue .comment-list .code-comment + .code-comment {
+    margin-top: 0.5rem !important;
+    padding-top: 0.5rem !important;
+  }
 }
 
 .repository.view.issue .comment-list .comment > .avatar {
@@ -1012,15 +1046,19 @@ pdf-object {
 }
 
 .repository.view.issue .comment-list .comment-code-cloud .segment.reactions {
-  margin-top: 16px !important;
+  gap: var(--button-spacing);
-  margin-bottom: -8px !important;
+  flex-wrap: wrap;
+  width: unset !important;
+  margin-left: calc(20px + 0.5rem) !important;
+  margin-top: 1rem !important;
+  margin-bottom: -1rem !important;
   border-top: none !important;
+  background: transparent !important;
 }
 
 .repository.view.issue .comment-list .comment-code-cloud .segment.reactions .ui.label {
   border: 1px solid;
   padding: 5px 8px !important;
-  margin: 0 2px;
   border-radius: var(--border-radius);
   border-color: var(--color-secondary-dark-1) !important;
 }
@@ -2031,7 +2069,7 @@ details.repo-search-result summary::marker {
   margin: 0 !important;
   position: relative;
   color: var(--color-text);
-  min-height: var(--repo-header-issue-min-height);
+  min-height: 41px;
   background-color: var(--color-box-header);
   display: flex;
   justify-content: space-between;
@@ -2081,11 +2119,11 @@ details.repo-search-result summary::marker {
   left: 7px;
 }
 
-.comment-header .actions a:not(.label) {
+.comment-header-right.actions a:not(.label) {
-  padding: 0.5rem !important;
+  padding: 0.5rem;
 }
 
-.comment-header .actions .label {
+.comment-header-right.actions .label {
   margin: 0 !important;
 }
 

web_src/css/review.css

@@ -57,13 +57,20 @@
   margin-bottom: 0.5em;
 }
 
+/* `.ui.label` implies `min-width: 0` breaking the layout.
+   (https://codeberg.org/forgejo/forgejo/pulls/8138#issuecomment-5960251)
+   When it becomes possible, feel free to replace with something better. */
+.collapsible-comment-box .ui.label {
+  min-width: unset;
+}
+
 .show-outdated:hover,
 .hide-outdated:hover {
   text-decoration: underline;
 }
 
 .comment-code-cloud {
-  padding: 0.5rem 1rem !important;
+  padding: 0.5rem !important;
   position: relative;
 }
 
@@ -71,10 +78,13 @@
   max-width: 820px;
 }
 
+.add-comment .comment-code-cloud form {
+  margin-left: 42px;
+}
+
 @media (max-width: 767.98px) {
   .comment-code-cloud {
     max-width: none;
-    padding: 0.75rem !important;
   }
 }
 
@@ -128,10 +138,6 @@
   cursor: pointer;
 }
 
-.comment-code-cloud .ui.active.tab {
-  padding: 0.5em;
-}
-
 .comment-code-cloud .ui.active.tab.markup {
   padding: 1em;
   min-height: 168px;
@@ -150,7 +156,7 @@
 }
 
 .comment-code-cloud .footer {
-  padding: 10px 0;
+  padding: 10px 0 0;
 }
 
 .comment-code-cloud .footer .markup-info {
@@ -171,7 +177,7 @@
 }
 
 .diff-file-body .comment-form {
-  margin: 0 0 0 3em;
+  margin-left: 3em;
 }
 
 .diff-file-body.binary {

web_src/css/themes/theme-forgejo-dark.css

@@ -309,11 +309,6 @@ i.grey.icon.icon.icon.icon {
 .ui.primary.labels .label {
   background-color: var(--color-primary-light-3) !important;
 }
-.repository.view.issue .comment-list .code-comment + .code-comment {
-  margin: 1.25rem 0 !important;
-  padding-top: 1.25rem !important;
-  border-top-color: var(--steel-650) !important;
-}
 .ui.labeled.icon.buttons > .button > .icon,
 .ui.labeled.icon.button > .icon {
   background-color: var(--color-light) !important;

web_src/css/themes/theme-forgejo-light.css

@@ -298,11 +298,6 @@
 .ui.primary.labels .label {
   background-color: var(--color-primary-dark-1) !important;
 }
-.repository.view.issue .comment-list .code-comment + .code-comment {
-  margin: 1.25rem 0 !important;
-  padding-top: 1.25rem !important;
-  border-top-color: var(--zinc-250) !important;
-}
 .ui.labeled.icon.buttons > .button > .icon,
 .ui.labeled.icon.button > .icon {
   background-color: var(--color-shadow) !important;