-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Misc icon tweaks, relax lint rules #36046
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
base: main
Are you sure you want to change the base?
Conversation
|
Regarding |
Maybe this icon is actually fine. I think a repo book icon with a padlock is better than a padlock only. We do not have a "locked" state yet for repos, so this icon is unambiguous too. |
|
One more minor change, |
|
A repository can fall into several state groups:
We can add icons for the states in Group 1, and use text labels for the other groups to maintain consistency and avoid visual conflicts. |
|
Sounds like a plan, I will try that. |
|
Applied that logic, so there is only 3 states of the repo icon now. Also, I disabled lint rules that forbid "else-return" constructs. I find those just annoying because they forbid code like this and fixing it breaks the symmetry of the block which looks ugly. } else {
return 'octicon-repo';
} |
Signed-off-by: silverwind <me@silverwind.io>
|
Ready now, I think it's a good cleanup and now all repo icons are the same everywhere. |
| {{else}} | ||
| <span class="icon">{{svg "octicon-repo"}}</span> | ||
| {{end}} | ||
| {{template "repo/icon" (dict "Repo" $repo "Size" 16)}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to have labels(private, template & etc.) for this repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean adding labels in this specific place (and below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it appears that the labels are only shown on the repository detail page. They could also be displayed on the list page, but I’m not sure whether we should enable that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lists we can add them, yes.
| {{else}} | ||
| {{svg "octicon-repo"}} | ||
| {{end}} | ||
| {{template "repo/icon" (dict "Repo" . "Size" 16)}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above
| {{else if $repo.IsMirror}} | ||
| <span class="icon">{{svg "octicon-mirror"}}</span> | ||
| {{else if $repo.IsTemplate}} | ||
| <span class="icon">{{svg "octicon-repo-template"}}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsTemplate is missing in icon.tmpl
If intentionally, should the comment #36046 (comment) be added to the template code? ps: I don't understand why IsTemplate is in a separate "group".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both mirror repositories and regular repositories(public or private) could be template repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If intentionally, should the comment #36046 (comment) be added to the template code? ps: I don't understand why IsTemplate is in a separate "group".
If intentionally, should the comment #36046 (comment) be added to the template code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it. And yes I've implemented that only group 1 is represented with the icon, all other groups are represented with tags in places that have enough room to display them (not on frontpage repo list).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought maybe we should make an exception for templates. I think it's highly unlikely that a repo is both a mirror and a template, so I guess showing a template icon in such a case would make more sense.
Useocticon-repo-lockedto indicate private repos, also visible on the repo header nowUseocticon-repo-forkedto indicate forked repos