-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 default avatar image size in PR diff page #28971
Fix default avatar image size in PR diff page #28971
Conversation
Can we use the new helper for the other two cases of
|
templates/repo/diff/comments.tmpl
Outdated
@@ -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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. The28
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:
- There are only a few (3?) cases, not too many, not reduce duplication really.
- It doesn't cover all cases, there will be still not only one
avatar_default.png
in other places. Developers still need to searchavatar_default.png
to avoid missing anything. - 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.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* giteaofficial/main: Add `must-change-password` cli parameter (go-gitea#27626) Include username in email headers (go-gitea#28981) Update tool dependencies (go-gitea#29030) Add artifacts v4 jwt to job message and accept it (go-gitea#28885) Pass es2020 to esbuild-loader as well (go-gitea#29027) Fix default avatar image size in PR diff page (go-gitea#28971) Update JS and PY dependencies, build for `es2020` browsers (go-gitea#28977)
Fix go-gitea#28941 ps: didn't test. The repo is too big to migrate.
Fix #28941
ps: didn't test. The repo is too big to migrate.