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

Expandable commit bodies #2980

Merged
merged 21 commits into from Nov 30, 2017

Conversation

7 participants
@sondr3
Contributor

sondr3 commented Nov 26, 2017

UPDATE: Here is how it looks right now:

e5998be9e0d7352e72b44f2918ce2f2a
f48f9d6f52849b35644d0b53791b964d

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

This comment has been minimized.

Member

lafriks commented Nov 26, 2017

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

This comment has been minimized.

Contributor

sondr3 commented Nov 26, 2017

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.

@sondr3 sondr3 force-pushed the sondr3:commit-message branch from 2388c31 to 67fde4d Nov 26, 2017

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

This comment has been minimized.

@lafriks

lafriks Nov 26, 2017

Member

just do

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

This comment has been minimized.

@sondr3

sondr3 Nov 26, 2017

Contributor

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

sondr3 added some commits Nov 26, 2017

Fixed proper newlines and minor refactor
Use <pre> instead of <p>, this is so we can use \n instead of having to manually place <br> into the HTML. Makes it easier to display commit bodies.
@lafriks

This comment has been minimized.

Member

lafriks commented Nov 26, 2017

Add comments to public functions

@sondr3

This comment has been minimized.

Contributor

sondr3 commented Nov 26, 2017

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?

screen shot 2017-11-26 at 23 41 27

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

This comment has been minimized.

Member

lafriks commented Nov 26, 2017

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

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

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 27, 2017

Member

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)

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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.

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

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 27, 2017

Member

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?

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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.

This comment has been minimized.

@lafriks

lafriks Nov 27, 2017

Member

Try:

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

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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

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

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 27, 2017

Member

Please indent with tabs

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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

@@ -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 comment has been minimized.

@ethantkoenig

ethantkoenig Nov 27, 2017

Member

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

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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

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

This comment has been minimized.

@lafriks

lafriks Nov 27, 2017

Member

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

This comment has been minimized.

@sondr3

sondr3 Nov 27, 2017

Contributor

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

sondr3 added some commits Nov 27, 2017

@sondr3

This comment has been minimized.

Contributor

sondr3 commented Nov 27, 2017

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 added some commits Nov 27, 2017

@sondr3

This comment has been minimized.

Contributor

sondr3 commented Nov 27, 2017

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.

@sondr3 sondr3 force-pushed the sondr3:commit-message branch from 04c2818 to f55878c Nov 28, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Nov 28, 2017

Codecov Report

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

Impacted file tree graph

@@            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 sondr3 changed the title from [WIP] Initial working state of expandable commit bodies to Expandable commit bodies Nov 28, 2017

@sondr3

This comment has been minimized.

Contributor

sondr3 commented Nov 28, 2017

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?

@sondr3 sondr3 referenced this pull request Nov 28, 2017

Closed

Multiple-Line commit message #2939

2 of 2 tasks complete

@lafriks lafriks added this to the 1.4.0 milestone Nov 28, 2017

@lafriks

This comment has been minimized.

Member

lafriks commented Nov 28, 2017

LGTM

@tboerger tboerger added lgtm/need 1 and removed lgtm/need 2 labels Nov 28, 2017

@lunny

This comment has been minimized.

Member

lunny commented Nov 29, 2017

LGTM

@tboerger tboerger added lgtm/done and removed lgtm/need 1 labels Nov 29, 2017

@lunny

This comment has been minimized.

Member

lunny commented Nov 29, 2017

@ethantkoenig needs your approval.

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

This comment has been minimized.

@ethantkoenig

ethantkoenig Nov 29, 2017

Member

Please indent with tabs

This comment has been minimized.

@sondr3

sondr3 Nov 29, 2017

Contributor

Will fix.

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

This comment has been minimized.

@techknowlogick

techknowlogick Nov 29, 2017

Member

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

This comment has been minimized.

@techknowlogick

techknowlogick Nov 29, 2017

Member

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

This comment has been minimized.

@lafriks

lafriks Nov 29, 2017

Member

@techknowlogick I think for this inline style is ok

This comment has been minimized.

@sondr3

sondr3 Nov 29, 2017

Contributor

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.

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

This comment has been minimized.

@techknowlogick

techknowlogick Nov 29, 2017

Member

same as above regarding inline CSS

@sondr3

This comment has been minimized.

Contributor

sondr3 commented Nov 29, 2017

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

This comment has been minimized.

Member

lunny commented Nov 29, 2017

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

@ethantkoenig

This comment has been minimized.

Member

ethantkoenig commented Nov 30, 2017

LGTM

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

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lafriks lafriks referenced this pull request Jan 29, 2018

Closed

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

2 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment