From cd08265406cc9e38b384b256aecdc463d4269664 Mon Sep 17 00:00:00 2001 From: 0ko <0ko@noreply.codeberg.org> Date: Wed, 27 Aug 2025 06:02:56 +0200 Subject: [PATCH] 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 --- templates/repo/diff/comment_form.tmpl | 14 +-- .../repo/issue/view_content/conversation.tmpl | 106 +++++++++--------- tests/e2e/pr-review.test.e2e.ts | 2 +- tests/integration/comment_roles_test.go | 69 +++++++----- tests/integration/pull_review_test.go | 6 +- web_src/css/repo.css | 70 +++++++++--- web_src/css/review.css | 22 ++-- web_src/css/themes/theme-forgejo-dark.css | 5 - web_src/css/themes/theme-forgejo-light.css | 5 - 9 files changed, 171 insertions(+), 128 deletions(-) diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index bb29583059..6dcba36c9e 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -1,5 +1,5 @@ {{if and $.root.SignedUserID (not $.Repository.IsArchived)}} -
+ {{$.root.CsrfTokenHtml}} @@ -25,22 +25,22 @@ {{end}} -
+
{{svg "octicon-markdown"}} {{ctx.Locale.Tr "repo.diff.comment.markdown_info"}} -
+
{{if $.reply}} - + {{else}} {{if $.root.CurrentReview}} - + {{else}} - + {{end}} {{end}} {{if or (not $.HasComments) $.hidden}} - + {{end}}
diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index a802d930fd..a472229302 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/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))}}
-
-
- {{(index .comments 0).TreePath}} +
+
+ {{(index .comments 0).TreePath}} {{if $invalid}} - + {{ctx.Locale.Tr "repo.issues.review.outdated"}} {{end}} @@ -49,64 +49,62 @@
{{end}}
-
- {{range .comments}} - {{$createdSubStr:= DateUtils.TimeSince .CreatedUnix}} -
-
-
-
- {{if not .OriginalAuthor}} - - {{ctx.AvatarUtils.Avatar .Poster 20}} - - {{end}} - - {{if .OriginalAuthor}} - - {{svg (MigrationIcon $.Repository.GetOriginalURLHostname)}} - {{.OriginalAuthor}} - - {{if $.Repository.OriginalURL}} - ({{ctx.Locale.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname}}) - {{end}} - {{else}} - {{template "shared/user/authorlink" .Poster}} + {{range .comments}} + {{$createdSubStr:= DateUtils.TimeSince .CreatedUnix}} +
+
+
+
+ {{if not .OriginalAuthor}} + + {{ctx.AvatarUtils.Avatar .Poster 20}} + + {{end}} + + {{if .OriginalAuthor}} + + {{svg (MigrationIcon $.Repository.GetOriginalURLHostname)}} + {{.OriginalAuthor}} + + {{if $.Repository.OriginalURL}} + ({{ctx.Locale.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname}}) {{end}} - {{ctx.Locale.Tr "repo.issues.commented_at" .HashTag $createdSubStr}} - -
-
- {{template "repo/issue/view_content/show_role" dict "ShowRole" .ShowRole "IsPull" $.Issue.IsPull}} - {{if not $.Repository.IsArchived}} - {{template "repo/issue/view_content/add_reaction" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID)}} - {{template "repo/issue/view_content/context_menu" dict "ctxData" $ "item" . "delete" true "issue" true "diff" true "IsCommentPoster" (and $.IsSigned (eq $.SignedUserID .PosterID))}} + {{else}} + {{template "shared/user/authorlink" .Poster}} {{end}} -
+ {{ctx.Locale.Tr "repo.issues.commented_at" .HashTag $createdSubStr}} +
-
-
- {{if .RenderedContent}} - {{.RenderedContent}} - {{else}} - {{ctx.Locale.Tr "repo.issues.no_content"}} - {{end}} -
-
{{.Content}}
-
- {{if .Attachments}} - {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} +
+ {{template "repo/issue/view_content/show_role" dict "ShowRole" .ShowRole "IsPull" $.Issue.IsPull}} + {{if not $.Repository.IsArchived}} + {{template "repo/issue/view_content/add_reaction" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID)}} + {{template "repo/issue/view_content/context_menu" dict "ctxData" $ "item" . "delete" true "issue" true "diff" true "IsCommentPoster" (and $.IsSigned (eq $.SignedUserID .PosterID))}} {{end}}
- {{$reactions := .Reactions.GroupByType}} - {{if $reactions}} - {{template "repo/issue/view_content/reactions" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID) "Reactions" $reactions}} +
+
+
+ {{if .RenderedContent}} + {{.RenderedContent}} + {{else}} + {{ctx.Locale.Tr "repo.issues.no_content"}} + {{end}} +
+
{{.Content}}
+
+ {{if .Attachments}} + {{template "repo/issue/view_content/attachments" dict "Attachments" .Attachments "RenderedContent" .RenderedContent}} {{end}}
+ {{$reactions := .Reactions.GroupByType}} + {{if $reactions}} + {{template "repo/issue/view_content/reactions" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID) "Reactions" $reactions}} + {{end}}
- {{end}} -
-
+
+ {{end}} +
{{if $resolved}}
diff --git a/tests/e2e/pr-review.test.e2e.ts b/tests/e2e/pr-review.test.e2e.ts index 577c8bf20a..03f3a3d8b3 100644 --- a/tests/e2e/pr-review.test.e2e.ts +++ b/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(); diff --git a/tests/integration/comment_roles_test.go b/tests/integration/comment_roles_test.go index fc9bc6f565..63b5d5f7cc 100644 --- a/tests/integration/comment_roles_test.go +++ b/tests/integration/comment_roles_test.go @@ -1,5 +1,5 @@ -// Copyright 2024 The Forgejo Authors. All rights reserved. -// SPDX-License-Identifier: MIT +// Copyright 2024-2025 The Forgejo Authors. All rights reserved. +// 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." - authorTooltipPR := "This user is the author of this pull request." - authorTooltipIssue := "This user is the author of this issue." - contributorTooltip := "This user has previously committed in this repository." - newContributorTooltip := "This is the first contribution of this user to the repository." + locale := translation.NewLocale("en-US") + authorTooltipPR := locale.TrString("repo.issues.author.tooltip.pr") + authorTooltipIssue := locale.TrString("repo.issues.author.tooltip.issue") + ownerTooltip := locale.TrString("repo.issues.role.owner_helper") + 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" - labels := commentHeads.Eq(0).Find(".role-label") + reviewHeads := page.Find(".timeline .code-comment .header .comment-header-right") + 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" - labels = commentHeads.Eq(5).Find(".role-label") + // Test the third regular comment labels + labels = commentHeads.Eq(3).Find(".role-label") assert.Equal(t, 1, labels.Length()) testIssueCommentUserLabel(t, labels.Eq(0), "First-time contributor", newContributorTooltip) }) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 67acfe6f00..35f287958c 100644 --- a/tests/integration/pull_review_test.go +++ b/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 }) } diff --git a/web_src/css/repo.css b/web_src/css/repo.css index 4aa6faa46e..ac671cf0ad 100644 --- a/web_src/css/repo.css +++ b/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 { - background: transparent; - border-bottom: 0 !important; - padding: 0 !important; +@media (max-width: 768px) { + .repository.view.issue .comment-list .code-comment { + padding: 0.33rem 0.33rem 1.33rem; + } } -.repository.view.issue .comment-list .code-comment .comment-header::after, -.repository.view.issue .comment-list .code-comment .comment-header::before { - display: none; +.repository.view.issue .comment-list .code-comment:target { + outline: 1px solid var(--color-primary); +} + +.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; - margin-bottom: -8px !important; + gap: var(--button-spacing); + 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) { - padding: 0.5rem !important; +.comment-header-right.actions a:not(.label) { + padding: 0.5rem; } -.comment-header .actions .label { +.comment-header-right.actions .label { margin: 0 !important; } diff --git a/web_src/css/review.css b/web_src/css/review.css index c1ef21bdc0..f815588b91 100644 --- a/web_src/css/review.css +++ b/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 { diff --git a/web_src/css/themes/theme-forgejo-dark.css b/web_src/css/themes/theme-forgejo-dark.css index b1b80510d4..94097497bf 100644 --- a/web_src/css/themes/theme-forgejo-dark.css +++ b/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; diff --git a/web_src/css/themes/theme-forgejo-light.css b/web_src/css/themes/theme-forgejo-light.css index 277b52165f..37ddc2a413 100644 --- a/web_src/css/themes/theme-forgejo-light.css +++ b/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;