Skip to content

Improve interface when comparing a branch which has created a pull request #17911

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

Merged
merged 6 commits into from
Dec 24, 2021
Merged

Improve interface when comparing a branch which has created a pull request #17911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1fe345b
Improve interface when comparing a branch which has created a pull re…
lunny Dec 5, 2021
88c7342
Take the note back
lunny Dec 22, 2021
8361adb
Merge branch 'main' into lunny/improve_push_compare
wxiaoguang Dec 23, 2021
539219b
Merge branch 'main' into lunny/improve_push_compare
zeripath Dec 23, 2021
e69338c
Merge branch 'main' into lunny/improve_push_compare
wxiaoguang Dec 24, 2021
e381d58
Merge branch 'main' into lunny/improve_push_compare
lunny Dec 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1429,6 +1429,7 @@ compare.compare_head = compare

pulls.desc = Enable pull requests and code reviews.
pulls.new = New Pull Request
pulls.view = View Pull Request
Copy link
Member

@delvh delvh Dec 5, 2021

Choose a reason for hiding this comment

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

According to your picture, I'm confused about what the PR shown is supposed to convey.
It's not immediately visible that that wants to show us that such a PR already exists.

I'd say there are two possible approaches here to avoid confusion:

  1. Explicitly make this string View already open Pull Request (at best combined with a name change)
    If we use this approach, do we even need repo.pulls.has_pull_request anymore?
  2. Display repo.pulls.has_pull_request (minus the link at the end) before displaying the open pull request.

Copy link
Member

@delvh delvh Dec 5, 2021

Choose a reason for hiding this comment

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

I just did a quick search using grep -irn.
According to that,

./routers/web/repo/pull.go:1387:                        errorMessage := ctx.Tr("repo.pulls.has_pull_request", html.EscapeString(ctx.Repo.RepoLink+"/pulls/"+strconv.FormatInt(err.IssueID, 10)), html.EscapeString(RepoRelPath), err.IssueID) // FIXME: Creates url insidde locale string

is the only other place that uses pulls.has_pull_request, which already does not seem particularly wanted.

What should we do about it then?

Copy link
Member Author

@lunny lunny Dec 5, 2021

Choose a reason for hiding this comment

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

The pull request maybe closed or merged, not always open state.

Copy link
Member Author

@lunny lunny Dec 5, 2021

Choose a reason for hiding this comment

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

It's not related with this PR so I left as it.

pulls.compare_changes = New Pull Request
pulls.compare_changes_desc = Select the branch to merge into and the branch to pull from.
pulls.compare_base = merge into
Expand Down
4 changes: 4 additions & 0 deletions routers/web/repo/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,10 @@ func CompareDiff(ctx *context.Context) {
}
} else {
ctx.Data["HasPullRequest"] = true
if err := pr.LoadIssue(); err != nil {
ctx.ServerError("LoadIssue", err)
return
}
ctx.Data["PullRequest"] = pr
ctx.HTML(http.StatusOK, tplCompareDiff)
return
Expand Down
18 changes: 16 additions & 2 deletions templates/repo/diff/compare.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,22 @@
{{end}}
{{else if and .PageIsComparePull (gt .CommitCount 0)}}
{{if .HasPullRequest}}
<div class="ui segment">
{{.i18n.Tr "repo.pulls.has_pull_request" (Escape $.RepoLink) (Escape $.RepoRelPath) .PullRequest.Index | Safe}}
<div class="ui segment grid title">
<div class="twelve wide column issue-title">
{{.i18n.Tr "repo.pulls.has_pull_request" (Escape $.RepoLink) (Escape $.RepoRelPath) .PullRequest.Index | Safe}}
<h1>
<span id="issue-title">{{RenderIssueTitle .PullRequest.Issue.Title $.RepoLink $.Repository.ComposeMetas}}</span>
<span class="index">#{{.PullRequest.Issue.Index}}</span>
</h1>
</div>
<div class="four wide right middle aligned column">
{{- if .PullRequest.HasMerged -}}
<a href="{{Escape $.RepoLink}}/pulls/{{.PullRequest.Issue.Index}}" class="ui button purple show-form">{{svg "octicon-git-merge" 16}} {{.i18n.Tr "repo.pulls.view"}}</a>
{{else if .Issue.IsClosed}}
<a href="{{Escape $.RepoLink}}/pulls/{{.PullRequest.Issue.Index}}" class="ui button red show-form">{{svg "octicon-issue-closed" 16}} {{.i18n.Tr "repo.pulls.view"}}</a>
{{else}}
<a href="{{Escape $.RepoLink}}/pulls/{{.PullRequest.Issue.Index}}" class="ui button green show-form">{{svg "octicon-git-pull-request" 16}} {{.i18n.Tr "repo.pulls.view"}}</a>
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 6, 2021

Choose a reason for hiding this comment

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

Will these purple/red/green work for dark theme? Otherwise LGTM.

Copy link
Member Author

@lunny lunny Dec 10, 2021

Choose a reason for hiding this comment

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

Can't get the result as red or purple. 🤔

{{end}}</div>
Comment on lines +199 to +205
Copy link
Member

@delvh delvh Dec 5, 2021

Choose a reason for hiding this comment

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

Do we need a template to render the PR icon?
I'd prefer so.

Copy link
Member Author

@lunny lunny Dec 5, 2021

Choose a reason for hiding this comment

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

If no other place can reuse it, I don't think we should move it to a new template. For different state of pull request, the color of the button and the icon are not different.

</div>
{{else}}
{{if and $.IsSigned (not .Repository.IsArchived)}}
Expand Down
9 changes: 9 additions & 0 deletions web_src/less/_repository.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,15 @@
.markup {
font-size: 14px;
}

.title {
.issue-title {
margin-bottom: .5rem;
.index {
color: var(--color-text-light-2);
}
}
}
}

.filter.dropdown .menu {
Expand Down