Skip to content

De-emphasize signed commits#31160

Merged
wxiaoguang merged 40 commits into
go-gitea:mainfrom
BlenderDefender:signed-commits-ui
Dec 27, 2024
Merged

De-emphasize signed commits#31160
wxiaoguang merged 40 commits into
go-gitea:mainfrom
BlenderDefender:signed-commits-ui

Conversation

@BlenderDefender
Copy link
Copy Markdown
Contributor

@BlenderDefender BlenderDefender commented May 29, 2024 •
edited by wxiaoguang
Loading

Update: the latest change still uses the old layout, the signed commits are still made to be de-emphasized a little (it won't affect its parents, for example: the commit details page), I didn't change their colors.

And the new code structure is easier to make more improvements or refactor, for example: change the colors to de-emphasize more, or design some new layouts.

screenshot

image

Old description:

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

Old Comparison

Latest commit

Before

grafik

After

grafik

Commit list

Before

grafik

After

grafik

Commit details

Before

grafik

After

grafik
grafik

Commit Graph

Before

grafik

After

grafik

Clicking on the badge opens the following modal:
grafik
grafik
grafik

@BlenderDefender
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 GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 29, 2024
BlenderDefender
BlenderDefender commented May 29, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
BlenderDefender
BlenderDefender commented May 29, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
@techknowlogick
Copy link
Copy Markdown
Member

techknowlogick commented May 29, 2024

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
Copy link
Copy Markdown
Contributor Author

BlenderDefender commented May 30, 2024

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

@silverwind
Copy link
Copy Markdown
Member

silverwind commented May 30, 2024 •
edited
Loading

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.

silverwind
silverwind reviewed May 30, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
silverwind
silverwind reviewed May 30, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
silverwind
silverwind reviewed May 30, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
silverwind
silverwind reviewed May 30, 2024
View reviewed changes
Comment thread templates/repo/signature_badge.tmpl Outdated
silverwind
silverwind reviewed May 30, 2024
View reviewed changes
Comment thread templates/repo/commits_list.tmpl Outdated
@BlenderDefender
Copy link
Copy Markdown
Contributor Author

BlenderDefender commented May 30, 2024

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

@silverwind
Copy link
Copy Markdown
Member

silverwind commented May 30, 2024 •
edited
Loading

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

@BlenderDefender
refactor: Remove classnames that don't have any effect
682a1e6 
This commit removes all classnames (like for example `ui text`) that don't have any effect on the elements they're applied to.
@silverwind
Copy link
Copy Markdown
Member

silverwind commented May 30, 2024 •
edited
Loading

One request: Instead of

image

could we use this SVG icon:

image

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.

@BlenderDefender
refactor: Remove isSigned styles from repo.css
f477bb4 
These styles aren't used anymore, so they can be safely deleted
@BlenderDefender
Copy link
Copy Markdown
Contributor Author

BlenderDefender commented May 30, 2024

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

@silverwind
Copy link
Copy Markdown
Member

silverwind commented May 30, 2024

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:

image

@lunny lunny added this to the 1.23.0 milestone May 31, 2024
@GiteaBot GiteaBot 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 Jun 12, 2024
@delvh
Copy link
Copy Markdown
Member

delvh commented Jun 20, 2024

@silverwind did you have time to fix these hacks?

@lunny lunny modified the milestones: 1.23.0, 1.24.0 Sep 24, 2024
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Dec 23, 2024

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?

@wxiaoguang
Merge branch 'main' into signed-commits-ui
2071013 
# Conflicts:
#	templates/repo/commit_page.tmpl
#	templates/repo/commits_list.tmpl
#	templates/repo/commits_list_small.tmpl
#	templates/repo/graph/commits.tmpl
#	templates/repo/latest_commit.tmpl
#	templates/repo/shabox_badge.tmpl
#	web_src/css/repo.css
@wxiaoguang
temp
aae3308
@wxiaoguang
fix
71a17ef
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Dec 27, 2024

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.

@wxiaoguang wxiaoguang marked this pull request as draft December 27, 2024 18:12
@wxiaoguang wxiaoguang marked this pull request as ready for review December 27, 2024 19:08
@GiteaBot GiteaBot 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 Dec 27, 2024
@wxiaoguang wxiaoguang mentioned this pull request Dec 27, 2024
Merged
@wxiaoguang wxiaoguang merged commit 079a1ff into go-gitea:main Dec 27, 2024
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Dec 27, 2024

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

zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 30, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
dcb2993 
* giteaofficial/main:
  Refactor pagination (go-gitea#33037)
  Test webhook email (go-gitea#33033)
  Fix review code comment avatar alignment (go-gitea#33031)
  Fix templating in pull request comparison (go-gitea#33025)
  Refactor tests (go-gitea#33021)
  [skip ci] Updated translations via Crowdin
  always show assignees on right (go-gitea#33006)
  fix toggle commit body button ui when latest commit message is long (go-gitea#32997)
  Fix and/or comment some legacy CSS problems (go-gitea#33015)
  Refactor comment history and fix content edit (go-gitea#33018)
  Fix bug on activities (go-gitea#33008)
  Refactor arch route handlers (go-gitea#32993)
  fix scoped label ui when contains emoji (go-gitea#33007)
  [skip ci] Updated translations via Crowdin
  De-emphasize signed commits (go-gitea#31160)
  Fix eslint (go-gitea#33002)
  Fix Agit pull request permission check (go-gitea#32999)
  Support for email addresses containing uppercase characters when activating user account (go-gitea#32998)
@wxiaoguang wxiaoguang mentioned this pull request Jan 1, 2025
Closed
@ghost ghost mentioned this pull request Jan 25, 2025
Closed
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@silverwind silverwind silverwind left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh approved these changes

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.

Projects

None yet

Milestone

1.24.0

Development

Successfully merging this pull request may close these issues.

De-emphasize signed commits

7 participants

@BlenderDefender @techknowlogick @silverwind @delvh @wxiaoguang @lunny @GiteaBot