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

Provide links in commit summaries in commits table/view list #7659

Merged
merged 6 commits into from
Aug 20, 2019
Merged

Provide links in commit summaries in commits table/view list #7659

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
195cb4f
Provide links in commit summaries in commits table/view list
gary-kim Jul 29, 2019
a88564b
Check that hashes are commits before making them links
gary-kim Jul 31, 2019
f21ec5e
Revert "Check that hashes are commits before making them links"
gary-kim Aug 16, 2019
879e876
Merge remote-tracking branch 'upstream/master' into enh/noid/links-in…
gary-kim Aug 16, 2019
d5f64cf
Add Commit Message Links to Files List
gary-kim Aug 19, 2019
fe82211
Merge remote-tracking branch 'upstream/master' into enh/noid/links-in…
gary-kim Aug 19, 2019
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
2 changes: 1 addition & 1 deletion templates/repo/commits_table.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
</td>
<td class="message">
<span class="message-wrapper">
<span class="commit-summary has-emoji{{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{.Summary}}</span>
<span class="commit-summary has-emoji{{if gt .ParentCount 1}} grey text{{end}}" title="{{.Summary}}">{{RenderCommitMessage .Message $.RepoLink $.Repository.ComposeMetas}}</span>
</span>
{{if IsMultilineCommitMessage .Message}}
<button class="basic compact mini ui icon button commit-button"><i class="ellipsis horizontal icon"></i></button>
Expand Down
6 changes: 3 additions & 3 deletions templates/repo/view_list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@
{{end}}
</a>
{{template "repo/commit_status" .LatestCommitStatus}}
<span class="grey has-emoji commit-summary" title="{{.LatestCommit.Summary}}">{{.LatestCommit.Summary}}
<span class="grey has-emoji commit-summary" title="{{.LatestCommit.Summary}}">{{RenderCommitMessage .LatestCommit.Message $.RepoLink $.Repository.ComposeMetas}}
{{if IsMultilineCommitMessage .LatestCommit.Message}}
<button class="basic compact mini ui icon button commit-button"><i class="ellipsis horizontal icon"></i></button>
<pre class="commit-body" style="display: none;">{{RenderCommitBody .LatestCommit.Message $.RepoLink $.Repository.ComposeMetas}}</pre>
{{end}}
</span>
</span>
</th>
<th class="text grey right age">{{if .LatestCommit.Author}}{{TimeSince .LatestCommit.Author.When $.Lang}}{{end}}</th>
</tr>
Expand Down Expand Up @@ -83,7 +83,7 @@
{{end}}
<td class="message">
<span class="truncate has-emoji">
<a href="{{$.RepoLink}}/commit/{{$commit.ID}}" title="{{$commit.Summary}}">{{$commit.Summary}}</a>
{{RenderCommitMessageLink $commit.Message $.RepoLink (Printf "%s/commit/%s" $.RepoLink $commit.ID) $.Repository.ComposeMetas}}
Copy link
Member

@lafriks lafriks Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is good idea to format this as there is not much point in this as links within links will add more confusion

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe what we need is different styling on the links inside links to make it more clear to users.

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly there was problems with links within links already in previous versions

Copy link
Member

Choose a reason for hiding this comment

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

imho this line change should be currently reverted until we come up with good solution for this

Copy link
Member Author

@gary-kim gary-kim Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What were the issues? I'd like to take a look as to whether they can be addressed.

EDIT: Whoops, your message didn't get updated when this comment was originally posted. Should I open a PR to revert that commit at least until all those issues you refer to have been ironed out? It is probably a good idea to address the issues in another branch but I'm not quite sure what the scope of the problems involving having links within links are yet.

Copy link
Member

Choose a reason for hiding this comment

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

First of all you can really tell apart where inner links begin and outer ends.
Need to check also if overflow hiding and elipsis adding does not breaks everyting if inner link happens to be at the end.

Yes, please open PR

Copy link
Member Author

Choose a reason for hiding this comment

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

@lafriks #7923

I think we can also use the PR to get some info about what the issues with the feature were.

</span>
</td>
<td class="text grey right age">{{TimeSince $commit.Committer.When $.Lang}}</td>
Expand Down