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

Introduce pull request commit view files page #26197

Closed
wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jul 28, 2023

This PR introduce a new page for view commit changes in a pull request. Before this PR, when click some commit in the commits list page of a pull request, it will jump to a commit view page. But now it will show in this pull request like GH did.

related to #25528
Before:

图片

After:

图片

This also avoids pull request commits have blank branches and tags listed.

图片

@lunny lunny added the type/enhancement An improvement of existing functionality label Jul 28, 2023
@lunny lunny added this to the 1.21.0 milestone Jul 28, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 28, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 28, 2023
@lunny
Copy link
Member Author

lunny commented Jul 28, 2023

CI failure looks like a template linter's bug.

@wxiaoguang
Copy link
Contributor

CI failure looks like a template linter's bug.

It's not a bug. It should avoid using such code layout

prInfo = PrepareViewPullInfo(ctx, issue)
}

if ctx.Written() {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. But it's still necessary because PrepareViewPullInfo is invoked.

@silverwind
Copy link
Member

Yes, it's a linter bug, but I'd also argue we should not conditionally create opening tags, there are better ways.

@silverwind
Copy link
Member

Actually no, this is not a linter bug in this case, you are actually outputting unbalanced tags, you need the same condition that is on the opening tag for a closing tag below.

@lunny
Copy link
Member Author

lunny commented Jul 31, 2023

I will rewrite this since #25528 merged and there are some conflicts.

@lunny lunny closed this Jul 31, 2023
@GiteaBot GiteaBot removed this from the 1.21.0 milestone Jul 31, 2023
@lunny lunny deleted the lunny/pr_view_commit branch July 31, 2023 01:21
silverwind pushed a commit that referenced this pull request Aug 13, 2023
…#26434)

Replace #26197
Since #25528 merged, the links of pull request commits should be
redirect to pull file changes UI but not the generic one.

---------

Co-authored-by: Giteabot <teabot@gitea.io>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants