gitea
gitea copied to clipboard
go-gitea
De-emphasize signed commits
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:
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.
@techknowlogick That would be really helpful. Do you know who I can ask to review this PR?
Looks nice based on the screenshots. I had started only on the commit detail page in https://github.com/go-gitea/gitea/pull/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.
@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.
@silverwind Looks like there are quite a few styles that will be removed with this... The
isSignedclass 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.
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.
Would look like this, I'm not quite sure about the warning icon in light mode though:
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:
Issue on commit graph: Unsigned commits have no spacing:
As I already mention, I strongly recommend to use tw-gap-2 on the flex parent, and then remove all horizontal margin classes tw-m* on all children. Please do that for all similar cases touched in this PR.
I pushed a change that introduces a commit-hash-link class used for plain hash links.
Commit graph:
Commit page:
Oh and one more thing: I assume there is more unused CSS, if you can identify more, please remove.
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.
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:
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-widthandinline-flexto 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.
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.
This whole commit message wrapping/ellipsis CSS is a huge mess that needs cleanup, I will see what I can do.
Okay I think I fixed all the overflows, here's all 4 variants of commit message now:
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.
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)
@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?
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.
I still think we can fix them without such hacks, I will check later but feel free to investigate yourself in the meanwhile.
@silverwind @BlenderDefender how to continue?
Manged to fix some problems. And FYI: the modal can't be used, the reason is complicated but since this PR is stale for long time, I won't spend time on explaining it unless there are interests.
Since this PR now also blocks others, so I merge it after CI passes. If there is anything wrong, I will also fix in first time.
And there could be more fine-tunes to the colors to de-emphasize more