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

Fix review code comment avatar alignment #33031

Merged

Conversation

henrygoodman
Copy link
Contributor

@henrygoodman henrygoodman commented Dec 29, 2024

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
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 29, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files 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
Contributor Author

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

ead17bb#commitcomment-150794153

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 29, 2024

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
Copy link
Contributor Author

henrygoodman commented Dec 29, 2024

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
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
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
Contributor

Maybe it should be as simple as:

{{if or .Content .Attachments}}

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

@henrygoodman
Copy link
Contributor Author

henrygoodman commented Dec 29, 2024

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

@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 added the backport/v1.23 This PR should be backported to Gitea 1.23 label Dec 29, 2024
@wxiaoguang wxiaoguang merged commit a96776b into go-gitea:main Dec 29, 2024
26 checks passed
@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 <[email protected]>
@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 <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/templates This PR modifies the template files size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review comment's avatars are not aligned
4 participants