Expandable commit bodies #2980

sondr3 · 2017-11-26T18:08:34Z

UPDATE: Here is how it looks right now:

Original post

I've taken some time to try to work on some kind of solution to the expandable commit message bodies that was talked about in #2939 and #2944, but trying to make it work like it does on GitHub and GitLab. It is however a very rough work in progress as I've never programmed in Go before nor have I much experience with JQuery.

There are several issues with this right now:

~~It shows the expand button for every commit, even if it's not required.~~
~~The commit bodies doesn't expand properly, the text get squished together in one line and I'm not sure how to display it over several lines~~
~~There's quite a bit of code duplication going on with the commit bodies~~

I'd like a helping hand with this to figure out how to best solve it, as I'm not a Go programmer.

lafriks · 2017-11-26T18:13:25Z

You should add function that checks if message contains newlines and returns true if it does and false otherwise. This way you can add:

{{if IsMultilineCommitMessage .Message }}
<button ....
<p class="commit-body" ....
{{end}}

sondr3 · 2017-11-26T21:08:41Z

Okay, I've fixed all the commit having the box for showing the commit body, now I've just gotta figure out how to properly render it in HTML.

lafriks · 2017-11-26T21:41:21Z

modules/templates/helper.go

+}
+
+func IsMultilineCommitMessage(msg string) bool {
+	msgLines := strings.Split(strings.TrimSpace(msg), "\n")


just do

return strings.Count(strings.TrimSpace(msg), "\n") > 1

Sweet, I was looking for a better way to do that.

Use <pre> instead of , this is so we can use \n instead of having to manually place into the HTML. Makes it easier to display commit bodies.

lafriks · 2017-11-26T22:46:14Z

Add comments to public functions

sondr3 · 2017-11-26T22:46:29Z

I've managed to make it place the commit bodies somewhat properly into the HTML now, my biggest hurdle now is getting the cells to stop moving when you expand the commit bodies. The image below shows what happens, when you press the [...] button it moves the commit time, avatar, username and SHA-code into the middle of the cell, which makes the whole layout jump around when you press it.

How would I stop that from happening?

Also, where do we display the commit messages in Gitea? I need to update all the places where we show the commit message to this new format.

@lafriks yeah, I haven't added them yet. I'll do it once I'm happy with them.

lafriks · 2017-11-26T22:48:34Z

To stop jumping you need to add vertical-align: top to css

ethantkoenig · 2017-11-27T03:58:33Z

modules/templates/helper.go

+	if len(body) == 0 {
+		return template.HTML("")
+	}
+	return template.HTML(strings.Join(body[1:], "\n"))


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  .~~ (the crossed-out part is incorrect, ignore it)

Yeah, I initially tried joining on   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.

ethantkoenig · 2017-11-27T03:59:47Z

public/js/index.js

@@ -2016,3 +2016,7 @@ function initFilterBranchTagDropdown(selector) {
        });
    });
 }
+
+$(".commit-button").click(function() {
+    $(this).next().toggle();


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?

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.

Try:

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

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

ethantkoenig · 2017-11-27T04:00:17Z

templates/repo/view_list.tmpl

@@ -28,6 +28,10 @@
 						{{end}}
 				</a>
 				<span class="grey has-emoji">{{RenderCommitMessage .LatestCommit.Summary .RepoLink $.Repository.ComposeMetas}}
+                {{if IsMultilineCommitMessage .LatestCommit.Message}}


Please indent with tabs

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

ethantkoenig · 2017-11-27T04:10:39Z

templates/repo/view_list.tmpl

@@ -28,6 +28,10 @@
 						{{end}}
 				</a>
 				<span class="grey has-emoji">{{RenderCommitMessage .LatestCommit.Summary .RepoLink $.Repository.ComposeMetas}}
+                {{if IsMultilineCommitMessage .LatestCommit.Message}}
+                <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>


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).

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

lafriks · 2017-11-27T10:34:11Z

modules/templates/helper.go

+	})
+}
+
+func RenderCommitBodyLink(msg, urlPrefix string, urlDefault string, metas map[string]string) template.HTML {


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

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

sondr3 · 2017-11-27T15:54:38Z

I've updated the code with the suggestions from you guys, but I also changed the vertical-align setting from top to baseline, otherwise the text gets ever so slightly bumped upwards.

edit: I'm not sure what the error is now, it suggests adding a newline to the CSS as far as I can see but that didn't work.

sondr3 · 2017-11-27T16:21:54Z

I've fixed a few other issues, however there are some things I want to change but I'm not sure whether they are out of the scope of this request, namely properly listing commits in issues and pull requests, nor whether I'll have time.

codecov-io · 2017-11-28T20:39:00Z

Codecov Report

Merging #2980 into master will decrease coverage by 0.05%.
The diff coverage is 13.33%.

@@            Coverage Diff             @@
##           master    #2980      +/-   ##
==========================================
- Coverage   33.03%   32.98%   -0.06%     
==========================================
  Files         270      270              
  Lines       39534    39549      +15     
==========================================
- Hits        13061    13044      -17     
- Misses      24617    24650      +33     
+ Partials     1856     1855       -1

Impacted Files	Coverage Δ
modules/templates/helper.go	`50.35% <13.33%> (-2.12%)`	⬇️
models/repo_indexer.go	`45.54% <0%> (-6.44%)`	⬇️
modules/process/manager.go	`76.81% <0%> (-4.35%)`	⬇️
modules/indexer/repo.go	`60.86% <0%> (-2.61%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cf90aa...f7592bb. Read the comment docs.

sondr3 · 2017-11-28T20:45:40Z

Alright, the tests are passing. I had to force push the latest commit again for it to properly test it for some reason, but hey. If anybody wants to give this a go I'd appreciate it! Should I squash the commits together?

lafriks · 2017-11-28T20:54:12Z

LGTM

lunny · 2017-11-29T01:48:00Z

LGTM

lunny · 2017-11-29T01:48:22Z

@ethantkoenig needs your approval.

ethantkoenig · 2017-11-29T01:50:54Z

public/less/_repository.less

@@ -1606,3 +1606,11 @@
 		}
 	}
 }
+
+.commit-list {
+    vertical-align: baseline;


Please indent with tabs

techknowlogick · 2017-11-29T02:45:48Z

templates/repo/commits_table.tmpl

@@ -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>


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?)

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

@techknowlogick I think for this inline style is ok

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.

techknowlogick · 2017-11-29T02:46:13Z

templates/repo/view_list.tmpl

@@ -28,6 +28,10 @@
 						{{end}}
 				</a>
 				<span class="grey has-emoji">{{RenderCommitMessage .LatestCommit.Summary .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>


same as above regarding inline CSS

sondr3 · 2017-11-29T08:17:20Z

Alright, fixed the spaces not being tabs. Given that this pull request contains some 20 commits, should I squash them together or just keep them as is?

lunny · 2017-11-29T08:32:22Z

@sondr3 just keep them as is. We will merge with squash.

ethantkoenig · 2017-11-30T04:47:47Z

LGTM

Initial working state of expandable commit bodies

f4a48d2

sondr3 mentioned this pull request Nov 26, 2017

2939 multiline commit message #2944

Closed

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 26, 2017

Fix all commits having showing button for multiline commits

67fde4d

sondr3 force-pushed the commit-message branch from 2388c31 to 67fde4d Compare November 26, 2017 21:12

lafriks requested changes Nov 26, 2017

View reviewed changes

sondr3 added 4 commits November 26, 2017 22:45

Refactor checking multiline messages method

524f957

Force newlines with in commit body

053cc1d

Show multiple lines in the list view of repositories

7afb927

Fixed proper newlines and minor refactor

23b92b8

Use <pre> instead of , this is so we can use \n instead of having to manually place into the HTML. Makes it easier to display commit bodies.

Fix commit list messages jumping around

7f25cd8

ethantkoenig suggested changes Nov 27, 2017

View reviewed changes

lafriks requested changes Nov 27, 2017

View reviewed changes

sondr3 added 5 commits November 27, 2017 16:47

Fix indentation in view_list.tmpl

65e6a19

Use vertical-align: baseline instead of top

ed572c2

Refactor commit button toggle function

3767880

Remove RenderCommitBodyLink function

2856f9e

Add comments

76e640d

sondr3 added 3 commits November 27, 2017 16:55

Add newline at the end of _repository.less

f325cf2

Fix long commit bodies not properly wrapping inside <pre>

a452635

Don't split on double newlines

a47c4c1

Show the commit body in commit view

8f02f61

Add/fix comments and run make fmt

f55878c

sondr3 force-pushed the commit-message branch from 04c2818 to f55878c Compare November 28, 2017 20:32

sondr3 changed the title ~~[WIP] Initial working state of expandable commit bodies~~ Expandable commit bodies Nov 28, 2017

sondr3 mentioned this pull request Nov 28, 2017

Multiple-Line commit message #2939

Closed

2 tasks

lafriks approved these changes Nov 28, 2017

View reviewed changes

lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 28, 2017

lafriks added this to the 1.4.0 milestone Nov 28, 2017

lafriks added type/enhancement An improvement of existing functionality and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Nov 28, 2017

tboerger 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 Nov 28, 2017

tboerger 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 Nov 29, 2017

ethantkoenig suggested changes Nov 29, 2017

View reviewed changes

techknowlogick reviewed Nov 29, 2017

View reviewed changes

Fix spaces not being tabs

083af01

ethantkoenig approved these changes Nov 30, 2017

View reviewed changes

Merge branch 'master' into commit-message

f7592bb

lunny merged commit 86ee41e into go-gitea:master Nov 30, 2017

lafriks mentioned this pull request Dec 17, 2017

Gitea only shows first line of commit message #3221

Closed

lafriks mentioned this pull request Jan 29, 2018

Cannot show multi-lines of comment in the commit #3427

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Expandable commit bodies #2980

Expandable commit bodies #2980

Conversation

sondr3 commented Nov 26, 2017 • edited Loading

UPDATE: Here is how it looks right now:

Original post

lafriks commented Nov 26, 2017

sondr3 commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Nov 26, 2017

sondr3 commented Nov 26, 2017 • edited Loading

lafriks commented Nov 26, 2017

ethantkoenig Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sondr3 commented Nov 27, 2017 • edited Loading

sondr3 commented Nov 27, 2017 • edited Loading

codecov-io commented Nov 28, 2017 • edited Loading

Codecov Report

sondr3 commented Nov 28, 2017 • edited Loading

lafriks commented Nov 28, 2017

lunny commented Nov 29, 2017

lunny commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sondr3 Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sondr3 commented Nov 29, 2017

lunny commented Nov 29, 2017

ethantkoenig commented Nov 30, 2017

sondr3 commented Nov 26, 2017 •

edited

Loading

sondr3 commented Nov 26, 2017 •

edited

Loading

ethantkoenig Nov 27, 2017 •

edited

Loading

sondr3 commented Nov 27, 2017 •

edited

Loading

sondr3 commented Nov 27, 2017 •

edited

Loading

codecov-io commented Nov 28, 2017 •

edited

Loading

sondr3 commented Nov 28, 2017 •

edited

Loading

sondr3 Nov 29, 2017 •

edited

Loading