Skip to content

Fix review code comment avatar alignment#33031

Merged
wxiaoguang merged 3 commits into
go-gitea:mainfrom
henrygoodman:fix-codecomment-avatar-alignment
Dec 29, 2024
Merged

Fix review code comment avatar alignment#33031
wxiaoguang merged 3 commits into
go-gitea:mainfrom
henrygoodman:fix-codecomment-avatar-alignment

Conversation

@henrygoodman

@henrygoodman henrygoodman commented Dec 29, 2024

Copy link
Copy Markdown
Contributor

Fixes #33017

Avatar should only have offset if the Comment has Content or Attachment to align with the speech bubble:

image

Otherwise, it should align with the timeline event badge.

Issue occurs when a comment is posted with Review.CodeComments but no Content.
Can be fixed by simply checking Content exists

Before:
broken_alignment

After:
fixed_alignment

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 29, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 29, 2024
@henrygoodman henrygoodman force-pushed the fix-codecomment-avatar-alignment branch from ead17bb to 71f704e Compare December 29, 2024 07:37
@henrygoodman

Copy link
Copy Markdown
Contributor Author

Sorry, did not see approval before force push, just small update for redundancy (functionally same I think)

ead17bb#commitcomment-150794153

Comment thread templates/repo/issue/view_content/comments.tmpl Outdated
@wxiaoguang

wxiaoguang commented Dec 29, 2024

Copy link
Copy Markdown
Contributor

I do no think this change is right complete, is it clear that why it doesn't match the CodeComments below?

Are all edge cases covered?

@henrygoodman

henrygoodman commented Dec 29, 2024

Copy link
Copy Markdown
Contributor Author

Are all edge cases covered?

I think cases needing to consider are combination of Review.Content and Review.CodeComments. Unsure if any other cases to test.

  1. Review.Content and Review.CodeComments

image

  1. not Review.Content and Review.CodeComments

image

  1. Review.Content and not Review.CodeComments

image

  1. not Review.Content and not Review.CodeComments

image

@wxiaoguang

Copy link
Copy Markdown
Contributor

I can see there is still an edge: leave a review comment with code comment, and edit the review comment to make it empty

image

@wxiaoguang

Copy link
Copy Markdown
Contributor

One more thing, by reading code, .Review.Content is not right here.

The rendered content is always .Content (issue comment), .Review.Content is never used.

For example, if you edit the review's content and save, the comment table's Content gets udpated, but the review table's Content doesn't, it is still the out-dated old content (never used).

@wxiaoguang

Copy link
Copy Markdown
Contributor

Maybe it should be as simple as:

{{if or .Content .Attachments}}

Because the "review" doesn't affect the "Content" rendering.

@henrygoodman

henrygoodman commented Dec 29, 2024

Copy link
Copy Markdown
Contributor Author

Maybe it should be as simple as:

{{if or .Content .Attachments}}

Because the "review" doesn't affect the "Content" rendering.

Good catch, thankyou. I guess Review and Review.Content must get transitively added at same time when Content is added but I guess if removing, it doesnt update the Review.Content. (so just checking Content is sufficient). Probably should have looked into the hierarchy more.

Tested and seems fine for edge cases

Comment thread templates/repo/issue/view_content/comments.tmpl Outdated
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 29, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) December 29, 2024 10:40
@wxiaoguang wxiaoguang merged commit a96776b into go-gitea:main Dec 29, 2024
@GiteaBot GiteaBot added this to the 1.24.0 milestone Dec 29, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 29, 2024
Fixes go-gitea#33017

Avatar should only have offset if the `Comment` has `Content` or
`Attachment` to align with the speech bubble.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Dec 29, 2024
wxiaoguang added a commit that referenced this pull request Dec 29, 2024
Backport #33031 by henrygoodman

Fixes #33017

Co-authored-by: Henry Goodman <79623665+henrygoodman@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 30, 2024
* giteaofficial/main:
  Refactor pagination (go-gitea#33037)
  Test webhook email (go-gitea#33033)
  Fix review code comment avatar alignment (go-gitea#33031)
  Fix templating in pull request comparison (go-gitea#33025)
  Refactor tests (go-gitea#33021)
  [skip ci] Updated translations via Crowdin
  always show assignees on right (go-gitea#33006)
  fix toggle commit body button ui when latest commit message is long (go-gitea#32997)
  Fix and/or comment some legacy CSS problems (go-gitea#33015)
  Refactor comment history and fix content edit (go-gitea#33018)
  Fix bug on activities (go-gitea#33008)
  Refactor arch route handlers (go-gitea#32993)
  fix scoped label ui when contains emoji (go-gitea#33007)
  [skip ci] Updated translations via Crowdin
  De-emphasize signed commits (go-gitea#31160)
  Fix eslint (go-gitea#33002)
  Fix Agit pull request permission check (go-gitea#32999)
  Support for email addresses containing uppercase characters when activating user account (go-gitea#32998)
@henrygoodman henrygoodman deleted the fix-codecomment-avatar-alignment branch February 6, 2025 10:10
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review comment's avatars are not aligned

4 participants