-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Refactor repo commit list #23690
Refactor repo commit list #23690
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
{{$commitLink:= printf "%s/commit/%s" $.comment.Issue.PullRequest.BaseRepo.Link (PathEscape .ID.String)}} | ||
|
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.
Why declare it so far above the first usage?
Why not below #34
?
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 think it's not that far. The code block looks like this:
{{$commitLink:=....}}
<span class="ui float right shabox">{{$commitLink}}</span> ## The Line 18
<span class="gt-mono commit-summar"..>{{$commitLink}}</span> ## The Line 50
So you can see that now commitLink
is at the same level of other blocks.
If we put it below line 34, it would look like
<span class="ui float right shabox">
{{$commitLink := ...}}
{{$commitLink}}
</span>
<span class="gt-mono commit-summar"..>
{{$commitLink}}
</span>
It looks like crossed the code blocks, it doesn't look good to me.
{{if $.comment.Issue.PullRequest.BaseRepo.Name}} | ||
<a href="{{$.comment.Issue.PullRequest.BaseRepo.Link}}/commit/{{PathEscape .ID.String}}" rel="nofollow" class="{{$class}}"> | ||
{{else}} | ||
<span class="{{$class}}"> |
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.
Under what condition was that block false?
I don't find any that makes sense, so I'd say this is a reasonable simplification…
* upstream/main: Fix issue due date edit toggle bug (go-gitea#23723) Fix profile page email display, respect settings (go-gitea#23747) Update Gitea version in docs (go-gitea#23755) Fix SVG close tag, improve commit graph page UI alignment (go-gitea#23751) Remove incorrect HTML self close tag (go-gitea#23748) Refactor repo commit list (go-gitea#23690) Fix tags view (go-gitea#23243) Add commit info in action page (go-gitea#23210) Use GitHub Actions compatible globbing for `branches`, `tag`, `path` filter (go-gitea#22804)
Before
if PullRequest.BaseRepo.Name
doesn't make sense, because the$commitLink
is always constructed belowif
blocks make the HTML tags (likely) not match in IDE. Although the rendered result matches, it's very unfriendly to editors or code analyzer, and it's difficult to read.After
Move the
$commitLink
assignment ahead.Simplify the code, resolve the above problems.