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: render job title as commit message #32748

Merged
merged 5 commits into from
Dec 8, 2024

Conversation

metiftikci
Copy link
Member

resolves #32724

i dont know if this is correct approach, waiting for suggestions

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 6, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/frontend labels Dec 6, 2024
@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Dec 6, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 7, 2024

To be honest, I do not think it is "correct". 🤣

  1. It shouldn't use RenderCommitMessage because issue-like title has its own render function, IIRC it is RenderIssueTitle
  2. I think it should avoid bypassing Vue framework , so attachRefIssueContextPopup is not needed if the gain is small.
    • attachRefIssueContextPopup is only used to attach a popup to some pure issue numbers in case there is no context: The issue #123 said ....
    • but here, the title already have enough related information
    • so I do not think attachRefIssueContextPopup is necessary, removing it would hugely improve code maintainability.

In my mind, the brief ideas are either:

  • Let backend render (RenderIssueTitle), frontend only use v-html and no need to do anything else
  • Let frontend render (add links to the title by regexp and element construction), then no need to touch backend or v-html

ps: you could use the mocked RepoActionView on devtest page to do test easily.

@metiftikci
Copy link
Member Author

Let backend render (RenderIssueTitle), frontend only use v-html and no need to do anything else

Are you sure to use RenderIssueTitle? Because what we see on the title is commit message

so I do not think attachRefIssueContextPopup is necessary, removing it would hugely improve code maintainability

Ok. I will remove issue context popup

Let frontend render (add links to the title by regexp and element construction), then no need to touch backend or v-html

Should i do it in this PR? Because as i see backend code more complex then to replace #number to link

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 7, 2024

Let backend render (RenderIssueTitle), frontend only use v-html and no need to do anything else

Are you sure to use RenderIssueTitle? Because what we see on the title is commit message

Then there is a RenderCommitMessageLinkSubject. TBH I haven't really looked into the problem and not sure which one is the best (has the best render behavior)

Update: maybe RenderCommitMessage is also right, just need to clarify the behavior

Let frontend render (add links to the title by regexp and element construction), then no need to touch backend or v-html

Should i do it in this PR? Because as i see backend code more complex then to replace #number to link

It depends, either frontend or backend should work (only one is enough)

But the question is whether it needs other render processors besides the link?

@wxiaoguang
Copy link
Contributor

Hmm sorry it seems that I misread, after reading the code again, RenderCommitMessage seems to be the right function to call

@metiftikci
Copy link
Member Author

@wxiaoguang done. is it ok. feel free to request anything else

@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 7, 2024
@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 7, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) December 8, 2024 02:17
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Dec 8, 2024
@wxiaoguang wxiaoguang added type/bug reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Dec 8, 2024
@wxiaoguang wxiaoguang merged commit ad99478 into go-gitea:main Dec 8, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 8, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 9, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Fix repo home row-right grow (go-gitea#32763)
  Refactor issue list (go-gitea#32755)
  Fix compare page bug view as anonymous (go-gitea#32754)
  Split issue/pull view router function as multiple smaller functions (go-gitea#32749)
  fix: render job title as commit message (go-gitea#32748)
  Fix typescript errors in Vue files, fix regression in "Recent Commits" chart (go-gitea#32649)
  Refactor LabelEdit (go-gitea#32752)
  [skip ci] Updated translations via Crowdin
  fix(project): add title to project view page (go-gitea#32747)
  [skip ci] Updated translations via Crowdin
  Fix case of .tsbuildinfo in .gitignore (go-gitea#32737)
  Support "merge upstream branch" (Sync fork) (go-gitea#32741)
  Update changelog to add missed changelog (go-gitea#32734)
  GitHub like repo home page (go-gitea#32213)
  Refactor markdown render (go-gitea#32736)
  Make wiki pages visit fast (go-gitea#32732)
  Refactor markdown render (go-gitea#32728)
  Refactor RepoActionView.vue, add `::group::` support (go-gitea#32713)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commit id not clickable in when viewing action log
5 participants