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.

Sign up for GitHub

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 default avatar image size in PR diff page #28971

Merged
merged 5 commits into from
Feb 2, 2024
Merged

Fix default avatar image size in PR diff page #28971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c823ee5
fix
yp05327 Jan 29, 2024
eb45970
improve
yp05327 Jan 30, 2024
3218862
fix
yp05327 Jan 30, 2024
a62b183
improve
yp05327 Jan 31, 2024
f57dbc4
Merge branch 'main' into fix-avatar-image-size-in-diff-page
GiteaBot Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions modules/templates/util_avatar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,9 @@ func (au *AvatarUtils) AvatarByEmail(email, name string, others ...any) template

return ""
}

// AvatarDefault renders default avatars.
func (au *AvatarUtils) AvatarDefault(others ...any) template.HTML {
size, class := gitea_html.ParseSizeAndClass(avatars.DefaultAvatarPixelSize, avatars.DefaultAvatarClass, others...)
return AvatarHTML(avatars.DefaultAvatarLink(), size, class, "")
}
2 changes: 1 addition & 1 deletion templates/repo/diff/comments.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{{$createdStr:= TimeSinceUnix .CreatedUnix ctx.Locale}}
<div class="comment" id="{{.HashTag}}">
{{if .OriginalAuthor}}
<span class="avatar"><img src="{{AppSubUrl}}/assets/img/avatar_default.png"></span>
<span class="avatar">{{ctx.AvatarUtils.AvatarDefault}}</span>
Copy link
Contributor

@wxiaoguang wxiaoguang Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think it's worth to add a new helper function.

It could be something as simple as:

<img class="ui avatar gt-vm" width="28" height="28" src="{{AppSubUrl}}/assets/img/avatar_default.png">

There are already code using <img class="ui avatar"> (tmpl/js) directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I did as you said. But for other maintainers. they may consider where does this 28 come from?
Maybe we can merge this in to AvatarUtils.Avatar, set item to be nil or something else?

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of the helper function. We use helper functions for all other avatars, why not this one too?

Copy link
Contributor

@wxiaoguang wxiaoguang Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The real situation here is:

  • There are still not only one avatar_default.png usages in JS code, not in tmpl, so your new "helper function" won't cover them.
  • If we'd like to improve the "default avatar" in the future, developers still to need search the whole code base, don't forget the JS ones.
  • Using <img class="ui avatar gt-vm" is also readable and it is standard HTML. The 28 is the default width, even if you use a helper function, in many uses you still need to write the side (you see, you use 40 now ..... you could also ask "where it comes from", the same)

So, introducing a helper function here seems an over-engineering IMO:

  1. There are only a few (3?) cases, not too many, not reduce duplication really.
  2. It doesn't cover all cases, there will be still not only one avatar_default.png in other places. Developers still need to search avatar_default.png to avoid missing anything.
  3. It needs more lines of code, but doesn't really help to improve the readability or maintainability, standard HTML code is also very readable and maintainable.
  4. The current approach introduces garbage code like <span class="avatar">

Instead of introducing a new helper function, the new commit which uses existing helper function seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you see, you use 40 now

It comes from:
image
At least for me, it can be easily considered that the avatars should be 40 here, but not the default size.

{{else}}
{{template "shared/user/avatarlink" dict "user" .Poster}}
{{end}}
Expand Down
Loading