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

Jump to bottom

Fix broken pull request files #22962

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Fix broken pull request files #22962

merged 3 commits into from
Feb 20, 2023

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 18, 2023

Fix #22961

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Feb 18, 2023
wxiaoguang
wxiaoguang reviewed Feb 18, 2023
View reviewed changes
templates/repo/diff/comments.tmpl
@@ -5,7 +5,7 @@
{{if .OriginalAuthor}}
<span class="avatar"><img src="{{AppSubUrl}}/assets/img/avatar_default.png"></span>
{{else}}
{{template "shared/user/avatarlink" Dict "Context" $.Context "user" .Poster}}
{{template "shared/user/avatarlink" Dict "Context" $.root.Context "user" .Poster}}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between Dict "Context" $.root.Context and Dict "ctx" $.root? I can see Dict "ctx" $.root below, which is using $.root as the ctx directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dict "Context" $.root.Context allows us to use $.Context in the underlying template.
Dict "ctx" $.root allows us to use $.ctx in the underlying template.

This is a data map, Dict could help to compose them into the data map.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that basic knowledge.

I mean What's the difference between Dict "Context" $.root.Context and Dict "ctx" $.root?

Why not use Dict "Context" $.root since the $.root is also a context used widely in this template?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we use Dict "Context" $.root, then we need $.Context.Context in the underlying template.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think I know the problem now.

$.root is a map (not a Context interface), Dict "ctx" $.root is passing the map as name ctx, it's not a real context.

The naming is quite misleading .......

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 18, 2023
@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 Feb 18, 2023
yardenshoham
yardenshoham reviewed Feb 18, 2023
View reviewed changes
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

I don't know if I have something wrong with my setup (using Gitpod) but I can't see the avatar:
image

Devtools:
image

PR page looks fine:
image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 18, 2023 •
edited
Loading

I don't know if I have something wrong with my setup (using Gitpod) but I can't see the avatar:

Because: the passed in item is not User, nor Collaborator, nor Organization. And there is no error message when the no case is matched (BUG, not related to your setup)

<a class="avatar"{{if gt .user.ID 0}} href="{{.user.HomeLink}}"{{end}}>{{avatar $.Context .}}</a>

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 19, 2023 •
edited
Loading

After

this fix could be right.

@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 Feb 19, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 19, 2023
@lunny lunny added this to the 1.19.0 milestone Feb 19, 2023
yardenshoham
yardenshoham approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

After merging with the latest main:
image

@lunny
Copy link
Member Author

lunny commented Feb 20, 2023

LG-TM bot

@lunny lunny merged commit 8e9814c into go-gitea:main Feb 20, 2023
@lunny lunny deleted the lunny/fix_broken_pr_files_comments branch February 20, 2023 01:57
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 20, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
049a04b 
* upstream/main:
  Fix broken pull request files (go-gitea#22962)
  Fix avatar misalignment (go-gitea#22955)
  Refactor the setting to make unit test easier (go-gitea#22405)
  Migration v244.go should be v243.go (go-gitea#22988)
  Adjust manifest to prevent tagging latest on rcs (go-gitea#22811)
  Add some guidelines for refactoring (go-gitea#22880)
  Rename `GetUnits` to `LoadUnits` (go-gitea#22970)
  Provide the ability to set password hash algorithm parameters (go-gitea#22942)
  Fix no user listed in org teams page (go-gitea#22979)
  Refactor hiding-methods, remove jQuery show/hide, remove `.hide` class, remove inline style=display:none (go-gitea#22950)
  Scoped labels (go-gitea#22585)
  Rename "People" to "Members" in organization page and use a better icon (go-gitea#22960)
  Rename `repo.GetOwner` to `repo.LoadOwner` (go-gitea#22967)
  Notify on container image create (go-gitea#22806)
  webview: Fix overflowing diff body (go-gitea#22959)
  Introduce customized HTML elements, fix incorrect AppUrl usages in templates (go-gitea#22861)
  Sort issues and pulls by recently updated in user and organization home (go-gitea#22925)
  Fix 404 error viewing the LFS file (go-gitea#22945)
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 20, 2023
This was referenced Feb 23, 2023
Merged
Merged
jolheiser pushed a commit that referenced this pull request Mar 2, 2023
@wxiaoguang
Refactor ctx in templates (#23105)
ce73492 
Before, the `dict "ctx" ...` map is used to pass data between templates.

Now, more and more templates need to use real Go context:

* #22962
* #23092


`ctx` is a Go concept for `Context`, misusing it may cause problems, and
it makes it difficult to review or refactor.

This PR contains 2 major changes:

* In the top scope of a template, the `$` is the same as the `.`, so the
old labels_sidebar's `root` is the `ctx`. So this `ctx` could just be
removed.
bd7f218
* Rename all other `ctx` to `ctxData`, and it perfectly matches how it
comes from backend: `"ctxData": ctx.Data`.
7c01260



From now on, there is no `ctx` in templates. There are only:

* `ctxData` for passing data
* `Context` for Go context
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@delvh delvh delvh approved these changes

@yardenshoham yardenshoham yardenshoham approved these changes

@wolfogre wolfogre wolfogre approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Milestone
1.19.0
Development

Successfully merging this pull request may close these issues.

Latest main breaks PRs with comments
6 participants
@lunny @wxiaoguang @wolfogre @yardenshoham @delvh @GiteaBot