Skip to content
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

fix duplicated icon in repolist #27627

Closed
wants to merge 1 commit into from

Conversation

denyskon
Copy link
Member

Fix #27596

Only show fork/mirror icon in title if the main icon is something else.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 14, 2023
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Oct 14, 2023
@denyskon denyskon requested a review from lng2020 October 14, 2023 20:15
@denyskon denyskon added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Oct 14, 2023
@denyskon denyskon added this to the 1.22.0 milestone Oct 14, 2023
@denyskon denyskon added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 14, 2023
@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 Oct 14, 2023
Copy link
Member

@lng2020 lng2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR

This PR will solve the problem but it's a little hacky. It uses the order of avatar icons, which is
confused when the reviewer didn't know about the sequence in

{{$avatarLink := (.RelAvatarLink ctx)}}
{{if $avatarLink}}
<img class="ui avatar gt-vm" src="{{$avatarLink}}" width="32" height="32" alt="{{.FullName}}">
{{else if $.IsTemplate}}
{{svg "octicon-repo-template" 32}}
{{else if $.IsPrivate}}
{{svg "octicon-lock" 32}}
{{else if $.IsMirror}}
{{svg "octicon-mirror" 32}}
{{else if $.IsFork}}
{{svg "octicon-repo-forked" 32}}
{{else}}
{{svg "octicon-repo" 32}}
{{end}}

Another design

Personally speaking, I think we should redesign those icons. I will share my opinion here but I won't be a blocker for this PR here since I have no preference for UI.

There are two kinds of labels after the repo name. The first one is the "label list", showing four kinds of repo: private, internal, private template, and internal template. I think it's a little weird because the private/internal and template are not so connected.

The second kind of label is the fork and mirror icons, which is pretty strange to me as to why those two icons are special. So I did some research and found out this pr.

The original code logic didn't split the fork and mirror, it just used else if to add the label. But this PR split the code. The author explained that why he doing this(#11891 (comment)), which is reasonable to me. Here is the quote.

Fork and mirror icon is kept, ideally in future we would display where repo is forked/mirrored from right on the explore page, like we do on repo header itself. We could then move mirror/fork icon along with source link to second line.

But before we add the original URL for the fork/mirror repo, the avatar icon is introduced. So it looks duplicated sometimes.

So here I want to propose a new design:

  1. The avatar icon includes four kinds: user-define repo avatar, mirror icon, fork icon, and default icon.
  2. A label to indicate whether it's private/internal.
  3. A label to indicate whether it's a template.
  4. A label to indicate whether it's an archive.

This design wouldn't hide information too because a repo can't both be the mirror(pull mirror) and fork.

(It seems that a fork of a template repo will not be a template either, not sure whether it's a bug.)
Edited: It's not a template after being forked, but it can be a template by changing settings

@denyskon
Copy link
Member Author

I think your proposal is reasonable. I'm on vacation currently, so I don't know whether I'll have the possibility to implement it in the next few days.

@denyskon denyskon closed this Oct 15, 2023
@GiteaBot GiteaBot removed this from the 1.22.0 milestone Oct 15, 2023
@lng2020
Copy link
Member

lng2020 commented Oct 15, 2023

I think your proposal is reasonable. I'm on vacation currently, so I don't know whether I'll have the possibility to implement it in the next few days.

Sorry to bother you on this. Happy vacation😄. I will try to implement it in the next few days.

@denyskon
Copy link
Member Author

Oh it's okay, but thanks :) Here is the new PR: #27644

6543 pushed a commit that referenced this pull request Oct 16, 2023
Fix #27596 

Change confusing behavior when showing information about a repo via
labels and icons. Implement changes proposed by @lng2020 in
#27627 (review).
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 16, 2023
Fix go-gitea#27596 

Change confusing behavior when showing information about a repo via
labels and icons. Implement changes proposed by @lng2020 in
go-gitea#27627 (review).
delvh pushed a commit that referenced this pull request Oct 19, 2023
Backport #27644 by @denyskon

Fix #27596 

Change confusing behavior when showing information about a repo via labels and icons.
Implement changes proposed by @lng2020 in
#27627 (review).

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 14, 2024
@lunny lunny removed the backport/v1.21 This PR should be backported to Gitea 1.21 label Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/templates This PR modifies the template files size/S Denotes a PR that changes 10-29 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The repo items in explore page have duplicate icon
5 participants