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

Expandable commit bodies #2980

Merged
merged 21 commits into from
Nov 30, 2017
Merged

Expandable commit bodies #2980

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f4a48d2
Initial working state of expandable commit bodies
sondr3 Nov 26, 2017
67fde4d
Fix all commits having showing button for multiline commits
sondr3 Nov 26, 2017
524f957
Refactor checking multiline messages method
sondr3 Nov 26, 2017
053cc1d
Force newlines with <br> in commit body
sondr3 Nov 26, 2017
7afb927
Show multiple lines in the list view of repositories
sondr3 Nov 26, 2017
23b92b8
Fixed proper newlines and minor refactor
sondr3 Nov 26, 2017
7f25cd8
Fix commit list messages jumping around
sondr3 Nov 26, 2017
65e6a19
Fix indentation in view_list.tmpl
sondr3 Nov 27, 2017
ed572c2
Use vertical-align: baseline instead of top
sondr3 Nov 27, 2017
3767880
Refactor commit button toggle function
sondr3 Nov 27, 2017
2856f9e
Remove RenderCommitBodyLink function
sondr3 Nov 27, 2017
76e640d
Add comments
sondr3 Nov 27, 2017
f325cf2
Add newline at the end of _repository.less
sondr3 Nov 27, 2017
a452635
Fix long commit bodies not properly wrapping inside <pre>
sondr3 Nov 27, 2017
a47c4c1
Don't split on double newlines
sondr3 Nov 27, 2017
8f02f61
Show the commit body in commit view
sondr3 Nov 27, 2017
63fec45
Update stylesheets
sondr3 Nov 27, 2017
e521e8a
Merge branch 'master' into commit-message
lafriks Nov 27, 2017
f55878c
Add/fix comments and run make fmt
sondr3 Nov 27, 2017
083af01
Fix spaces not being tabs
sondr3 Nov 29, 2017
f7592bb
Merge branch 'master' into commit-message
lunny Nov 30, 2017
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
33 changes: 33 additions & 0 deletions modules/templates/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ func NewFuncMap() []template.FuncMap {
},
"RenderCommitMessage": RenderCommitMessage,
"RenderCommitMessageLink": RenderCommitMessageLink,
"RenderCommitBody": RenderCommitBody,
"RenderCommitBodyLink": RenderCommitBodyLink,
"IsMultilineCommitMessage": IsMultilineCommitMessage,
"ThemeColorMetaTag": func() string {
return setting.UI.ThemeColorMetaTag
},
Expand Down Expand Up @@ -278,6 +281,36 @@ func renderCommitMessage(msg string, opts markup.RenderIssueIndexPatternOptions)
return template.HTML(msgLines[0])
}

func RenderCommitBody(msg, urlPrefix string, metas map[string]string) template.HTML {
return renderCommitBody(msg, markup.RenderIssueIndexPatternOptions{
URLPrefix: urlPrefix,
Metas: metas,
})
}

func RenderCommitBodyLink(msg, urlPrefix string, urlDefault string, metas map[string]string) template.HTML {
Copy link
Member

Choose a reason for hiding this comment

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

This function does not seems to be used anywhere, so should be dropped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I just copied the RenderCommit functions and changed them a tiny bit.

return renderCommitMessage(msg, markup.RenderIssueIndexPatternOptions{
DefaultURL: urlDefault,
URLPrefix: urlPrefix,
Metas: metas,
})
}

func renderCommitBody(msg string, opts markup.RenderIssueIndexPatternOptions) template.HTML {
cleanMsg := template.HTMLEscapeString(msg)
fullMessage := string(markup.RenderIssueIndexPattern([]byte(cleanMsg), opts))
msgLines := strings.Replace(strings.TrimSpace(fullMessage), "\n\n", "\n", -1)
body := strings.Split(msgLines,"\n")
if len(body) == 0 {
return template.HTML("")
}
return template.HTML(strings.Join(body[1:], "\n"))
Copy link
Member

@ethantkoenig ethantkoenig Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should differentiate between single and double new-lines, since a single newline may just be wrapping a long line, but a double newline typically indicates the start of a new paragraph. In fact, I think the best solution is just to render newlines as they appear in the commit message (this is what Github does).

Also, note that newlines in a template.HTML will not be rendered as newlines. The somewhat hacky way to fix this problem is to replace newlines with <br>. (the crossed-out part is incorrect, ignore it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I initially tried joining on <br> but then I looked at Github and saw that they wrapped their commit messages in <pre>. However, if that's the case I think the method needs to work differently because it already splits on \n and then I discard the first element and join the others. It seems to render correctly, but I'll see what I can figure out.

}

func IsMultilineCommitMessage(msg string) bool {
return strings.Count(strings.TrimSpace(msg), "\n") > 1
}

// Actioner describes an action
type Actioner interface {
GetOpType() models.ActionType
Expand Down
4 changes: 4 additions & 0 deletions public/js/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,3 +2016,7 @@ function initFilterBranchTagDropdown(selector) {
});
});
}

$(".commit-button").click(function() {
$(this).next().toggle();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a JS expert, but this seems fragile. What if we add a DOM element between the commit button and the commit body? Perhaps it would be better to select for the commit-body class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing

$(".commit-button").click(function() {
    $("commit-body").toggle();

But that opens and closes all of the commit messages, not just the one selected. All the documentation I could find from both JQuery themselves and online used this method, otherwise I think we'd have to use one of those JQuery-Toggle libraries.

Copy link
Member

Choose a reason for hiding this comment

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

Try:

$(".commit-button").on('click', function() {
  $(this).parent().find('.commit-body').toggle();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll give it a go later today.

});
4 changes: 4 additions & 0 deletions public/less/_repository.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1606,3 +1606,7 @@
}
}
}

.commit-list {
vertical-align: top;
}
6 changes: 5 additions & 1 deletion templates/repo/commits_table.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
<th class="three wide right aligned">{{.i18n.Tr "repo.commits.date"}}</th>
</tr>
</thead>
<tbody>
<tbody class="commit-list">
{{ $r:= List .Commits}}
{{range $r}}
<tr>
Expand Down Expand Up @@ -61,6 +61,10 @@
</td>
<td class="message collapsing">
<span class="has-emoji{{if gt .ParentCount 1}} grey text{{end}}">{{RenderCommitMessage .Summary $.RepoLink $.Repository.ComposeMetas}}</span>
{{if IsMultilineCommitMessage .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 .Message $.RepoLink $.Repository.ComposeMetas}}</pre>
Copy link
Member

Choose a reason for hiding this comment

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

There is inline CSS here, could you change it out with a CSS class?
Also this requires JS which means if someone has JS disabled then they won't be able to see the long commit message (perhaps default to open when JS is disabled?)

Copy link
Member

Choose a reason for hiding this comment

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

Or perhaps hide the button if the user doesn't have JS is another option

Copy link
Member

Choose a reason for hiding this comment

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

@techknowlogick I think for this inline style is ok

Copy link
Contributor Author

@sondr3 sondr3 Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it's not the prettiest, but the toggle function changes the style inline so I thought it was a decent compromise. Regarding what to do when JS is disabled, I'll leave that to the Gitea maintainers, I think Gitea depends on JS enough that if you disable it you're on your own.

{{end}}
{{template "repo/commit_status" .Status}}
</td>
<td class="grey text right aligned">{{TimeSince .Author.When $.Lang}}</td>
Expand Down
4 changes: 4 additions & 0 deletions templates/repo/view_list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
{{end}}
</a>
<span class="grey has-emoji">{{RenderCommitMessage .LatestCommit.Summary .RepoLink $.Repository.ComposeMetas}}
{{if IsMultilineCommitMessage .LatestCommit.Message}}
Copy link
Member

Choose a reason for hiding this comment

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

Please indent with tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it, Golands for some reason doesn't want to respect the settings.

<button class="basic compact mini ui icon button commit-button"><i class="ellipsis horizontal icon"></i></button>
<p class="commit-body" style="display: none;">{{RenderCommitBody .LatestCommit.Message $.RepoLink $.Repository.ComposeMetas}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

This should not be inside a paragraph, but instead inside a <pre> like in the other template (in order for newlines to be rendered properly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I forgot to change this one once I updated the other.

{{end}}
{{template "repo/commit_status" .LatestCommitStatus}}</span>
</th>
<th class="nine wide">
Expand Down