De-emphasize signed commits #31160

BlenderDefender · 2024-05-29T12:52:25Z

This pull request changes how signed commits are displayed, as signed commits were emphasized too much previously. This includes a rewrite of the commit list UI, which now uses an unordered list instead of a table. If a commit is signed, a clickable badge is shown, indicating that the commit is signed with either a verified or an unverified signature.

fixes #29641

Comparison:

Latest commit

Before

After

Commit list

Before

After

Commit details

Before

After

Commit Graph

Before

After

Clicking on the badge opens the following modal:

Rework the signed commits UI to show a clickable badge for signed commits. Clicking this badge reveals further information about the trust status of the signature.

templates/repo/signature_badge.tmpl

techknowlogick · 2024-05-29T23:40:07Z

Thanks!! 🙏

This is so helpful. The visual changes look great!

I think the CI changes may be related, perhaps due to an undefined variable previously wrapped in a guard conditional.

My main development machine is undergoing some repairs, and so I can't properly go through this changeset for review right now, but if you'd like for me to determine the cause of the CI fail I can do that for you once I'm back on my main device.

BlenderDefender · 2024-05-30T07:32:28Z

@techknowlogick That would be really helpful. Do you know who I can ask to review this PR?

silverwind · 2024-05-30T16:05:06Z

Looks nice based on the screenshots. I had started only on the commit detail page in #30221, but this looks more complete. Maybe you can take some inspiration from my PR.

You will definitely want to include the removal of unused CSS here.

templates/repo/signature_badge.tmpl

templates/repo/commits_list.tmpl

BlenderDefender · 2024-05-30T19:49:37Z

@silverwind Looks like there are quite a few styles that will be removed with this... The isSigned class isn't used anymore, so all styles with that selector can be removed.

… the locale file

silverwind · 2024-05-30T20:14:37Z

@silverwind Looks like there are quite a few styles that will be removed with this... The isSigned class isn't used anymore, so all styles with that selector can be removed.

Indeed, please remove all CSS that has the isSigned class in it. I did similar here but am not sure if my removal there is complete, so remove until a search for isSigned does not match anywhere in the CSS files anymore.

This commit removes all classnames (like for example `ui text`) that don't have any effect on the elements they're applied to.

silverwind · 2024-05-30T20:19:14Z

One request: Instead of

could we use this SVG icon:

If you want to show some additional text, it can be done with data-tooltip-content on the element. Again, you can take a hint from my PR, the icon logic is here and we should extract it into a subtemplate.

These styles aren't used anymore, so they can be safely deleted

BlenderDefender · 2024-05-30T20:35:05Z

Would look like this, I'm not quite sure about the warning icon in light mode though:

silverwind · 2024-05-30T20:42:58Z

Hmm, after seeing GitHub also has only text labels in these places, let's keep the text labels. One change I would like to see is normal border radius (tw-rounded), then I think it's fine. Should look similar to this:

…ls modal) by using flex-wrap

BlenderDefender · 2024-05-31T19:15:11Z

I'm trying to find the causes of the failing tests, here's what I've found:
tests/integration/git_test.go:628, tries to find a tbody that doesn't exist anymore
tests/integration/pull_status_test.go:48, 86 and 91, same issue as above
tests/integration/repo_commits_search_test.go:25, same issue as above

And the tests in repo_commits_test.go still fail, unfortunately.

BlenderDefender · 2024-05-31T19:40:33Z

I guess this needs to be fixed... It would be great, if the backend processed the commit summary in a way, that the summary has a maximum length, and everything longer than that would be added to the multiline commit message. For now, I'm fixing this by adding max-width and inline-flex to the commit summary:

silverwind · 2024-05-31T21:31:44Z

I guess this needs to be fixed... It would be great, if the backend processed the commit summary in a way, that the summary has a maximum length, and everything longer than that would be added to the multiline commit message. For now, I'm fixing this by adding max-width and inline-flex to the commit summary:

We tend to fix such issues in the frontend only, and I guess in the case of commit list we could just make it wrap for now which matches the previous implementation.

Commit list wraps
Single commit view wraps
Files table uses ellipsis
Latest commit to file uses ellsipsis

Maybe unify later, backend already has a mechanism for the "..." button for the commit message body.

silverwind · 2024-05-31T21:33:52Z

For now, I'm fixing this by adding max-width and inline-flex to the

max-width is never the right fix, but I will try if I come up with something better, there are many methods to fix overflow, most involve gt-ellipsis and restricting the parent node width, but in this case we should use wrap anyways.

silverwind · 2024-05-31T22:01:25Z

This whole commit message wrapping/ellipsis CSS is a huge mess that needs cleanup, I will see what I can do.

silverwind · 2024-05-31T22:28:11Z

Okay I think I fixed all the overflows, here's all 4 variants of commit message now:

silverwind · 2024-05-31T23:21:23Z

And I also fixed the commit message expansion on .latest-commit:

Unexpanded:

Expanded:

Not perfect, but acceptable imho, given that we are still have the restricting <table> in the repo files table.

templates/repo/signature_badge.tmpl

delvh · 2024-06-02T15:58:40Z

templates/repo/signature_badge.tmpl

+			{{if .verification.Verified}}
+				{{if ne .verification.SigningUser.ID 0}}
+					{{if .verification.SigningSSHKey}}
+						<span>{{ctx.Locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
+						{{.verification.SigningSSHKey.Fingerprint}}
+					{{else}}
+						<span>{{ctx.Locale.Tr "repo.commits.gpg_key_id"}}:</span>
+						{{.verification.SigningKey.PaddedKeyID}}
+					{{end}}
+				{{else}}
+					{{if .verification.SigningSSHKey}}
+						<span data-tooltip-content="{{ctx.Locale.Tr "gpg.default_key"}}">{{ctx.Locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
+						{{.verification.SigningSSHKey.Fingerprint}}
+					{{else}}
+						<span data-tooltip-content="{{ctx.Locale.Tr "gpg.default_key"}}">{{ctx.Locale.Tr "repo.commits.gpg_key_id"}}:</span>
+						{{.verification.SigningKey.PaddedKeyID}}
+					{{end}}
+				{{end}}
+			{{else if .verification.Warning}}
+				{{if .verification.SigningSSHKey}}
+					<span>{{ctx.Locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
+					{{.verification.SigningSSHKey.Fingerprint}}
+				{{else}}
+					<span>{{ctx.Locale.Tr "repo.commits.gpg_key_id"}}:</span>
+					{{.verification.SigningKey.PaddedKeyID}}
+				{{end}}
+			{{else}}
+				{{if .verification.SigningKey}}
+					{{if ne .verification.SigningKey.KeyID ""}}
+						<span>{{ctx.Locale.Tr "repo.commits.gpg_key_id"}}:</span>
+						{{.verification.SigningKey.PaddedKeyID}}
+					{{end}}
+				{{end}}
+				{{if .verification.SigningSSHKey}}
+					{{if ne .verification.SigningSSHKey.Fingerprint ""}}
+						<span>{{ctx.Locale.Tr "repo.commits.ssh_key_fingerprint"}}:</span>
+						{{.verification.SigningSSHKey.Fingerprint}}
+					{{end}}
+				{{end}}
+			{{end}}


Wait, how do all these spans differ?
To me, it looks like there's a lot of unnecessary duplication here.

That's honestly a good question, I've pretty much just copied over the code from templates/repo/commit_page.tmpl and adapted the layout to fit into the modal. I'll try to find ways to remove duplication here.

silverwind · 2024-06-02T17:50:18Z

For reference, here are the currently failing tests. Failing very likely because of changed HTML structure.

FAIL: TestGit (132.41s)
FAIL: TestGit/HTTP (58.30s)
FAIL: TestGit/HTTP/AutoMerge (3.54s)
FAIL: TestGit/HTTP/AutoMerge/CreateStatus (0.10s)
FAIL: TestGit/HTTP/AutoMerge/CreateStatus#01 (0.09s)
FAIL: TestGit/HTTP/AutoMerge/CreateStatus#02 (0.09s)
FAIL: TestPullCreate_CommitStatus (3.12s)
FAIL: TestPullCreate_CommitStatus/CreateStatus (0.09s)
FAIL: TestPullCreate_CommitStatus/CreateStatus#01 (0.10s)
FAIL: TestPullCreate_CommitStatus/CreateStatus#02 (0.09s)
FAIL: TestPullCreate_CommitStatus/CreateStatus#03 (0.10s)
FAIL: TestPullCreate_CommitStatus/CreateStatus#04 (0.10s)
FAIL: TestRepoCommitsSearch (1.37s)
FAIL: TestRepoCommitsWithStatusPending (1.04s)
FAIL: TestRepoCommitsWithStatusSuccess (1.05s)
FAIL: TestRepoCommitsWithStatusError (1.02s)
FAIL: TestRepoCommitsWithStatusFailure (1.04s)
FAIL: TestRepoCommitsWithStatusWarning (1.05s)
FAIL: TestRepoCommitsStatusMultiple (1.07s)

BlenderDefender · 2024-06-09T09:52:02Z

@silverwind Regarding the failing tests, would it be an option to add a data-testid attribute to all the elements that the tests are trying to check?

silverwind · 2024-06-11T11:52:38Z

Littering templates with test attributes is not acceptable imho. I still think we can fix them without such hacks, I will check later but feel free to investigate yourself in the meanwhile.

delvh · 2024-06-20T20:16:28Z

@silverwind did you have time to fix these hacks?

feat: De-emphasize signed commits

7c4f9bf

Rework the signed commits UI to show a clickable badge for signed commits. Clicking this badge reveals further information about the trust status of the signature.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 29, 2024

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2024

github-actions bot added the modifies/templates This PR modifies the template files label May 29, 2024

BlenderDefender commented May 29, 2024

View reviewed changes