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

Add org avatar on top of internal repo icon #11895

Merged
merged 12 commits into from
Jun 26, 2020

Conversation

CirnoT
Copy link
Contributor

@CirnoT CirnoT commented Jun 15, 2020

chrome_2020-06-15_14-15-19

Required for #11889 as there is no longer specific icon for internal repos in set.

This was referenced Jun 15, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 15, 2020
@silverwind
Copy link
Member

silverwind commented Jun 15, 2020

Not a fan of these negative margins. How about adding a wrapper <div> around the icon, set position: relative on it and then position the image position: absolute; right: 0; bottom: 0?

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

Adding another div element will cause repo name to flow down, requiring adding flex on headcrumb container with additional padding for svg element.

Also setting 0,0 positions avatar at center of octicon due to styling applied on .repository .repo-header .ui.huge.breadcrumb svg. But even without that the position is not ideal and I'd have to apply -5px on right

@silverwind
Copy link
Member

Adding another div element will cause repo name to flow down

Could just display: inline-block it, but ideal solution should be flexbox which would allow proper centering of repo icon and text without any pixel offsets.

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

Could just display: inline-block it

Nope. Would prefer not playing too much with flexbox there, also text is not supposed to be centered there.

@silverwind
Copy link
Member

inline-block div wrapper gives exactly the same rendering, you should try it:

image

The icon hanging outside the parent is a consequence of its size and seen present without a wrapper:

image

I'd say size down the icon a bit so it does not hang outside the parent and then use absolute positioning like I mentioned above with the wrapper.

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

The icon must be in multiplies of 16px otherwise it'll it might appear blurry, we can't willy nilly set any width and height on svgs, the world doesn't work like that.

FWIW the internal repo icon is squashed anyway because it's native width is 13px

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

This is the best I can do without negative margins:

Index: templates/repo/header_icon.tmpl
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- templates/repo/header_icon.tmpl	(revision c96107079f4c12f0ee81443b1eaccd24d85ae72e)
+++ templates/repo/header_icon.tmpl	(date 1592237893537)
@@ -9,8 +9,10 @@
 		{{svg "octicon-lock" 32}}
 	{{else if and (not $.IsMirror) (not $.IsFork) ($.Owner)}}
 		{{if $.Owner.Visibility.IsPrivate}}
-			{{svg "octicon-internal-repo" 32}}
-			<img class="ui avatar image" src="{{$.Owner.RelAvatarLink}}">
+			<div class="repo-owner-avatar">
+				{{svg "octicon-internal-repo" 32}}
+				<img class="ui avatar image" src="{{$.Owner.RelAvatarLink}}">
+			</div>
 		{{else}}
 			{{svg "octicon-repo" 32}}
 		{{end}}
Index: web_src/less/_repository.less
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- web_src/less/_repository.less	(revision c96107079f4c12f0ee81443b1eaccd24d85ae72e)
+++ web_src/less/_repository.less	(date 1592238260448)
@@ -15,19 +15,19 @@
             font-weight: 400;
             font-size: 1.5rem;
 
-            svg {
+            &.repo-title .repo-owner-avatar {
+                display: inline-block;
                 position: relative;
-                top: 5px;
-            }
 
-            &.repo-title .avatar {
-                margin-left: -23px;
-                color: #fafafa;
-                box-shadow: 0 0 0 2px;
-                width: 16px;
-                height: 16px;
-                margin-bottom: -30px;
-                border-radius: 2px;
+                .avatar {
+                    position: absolute;
+                    right: -5px;
+                    bottom: 0;
+                    width: 16px;
+                    height: 16px;
+                    color: #fafafa;
+                    box-shadow: 0 0 0 2px;
+                }
             }
 
             svg.octicon-lock {

This does however change how icon is presented along with title:
chrome_2020-06-15_18-25-59

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

Actually managed to eliminate need for -5px by applying margin: 0 on image.

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

Checked and looks fine to me on all cases, including when avatar is used instead of icon, so should be fine. Pushed changes.

@CirnoT
Copy link
Contributor Author

CirnoT commented Jun 15, 2020

Looking again at v1.11, it seems that top: 5px is completely unnecessary now and actually breaks styling in v1.12:

v1.11:
chrome_2020-06-15_20-26-14

v1.12:
chrome_2020-06-15_20-27-32

This PR:
chrome_2020-06-15_20-29-09

@silverwind
Copy link
Member

we can't willy nilly set any width and height on svgs

Must be an issue on your GPU or something. I never have any blurriness issues with non-integer scaled SVG on multiple machines.

@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 15, 2020
@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Jun 15, 2020
@techknowlogick techknowlogick added this to the 1.13.0 milestone Jun 25, 2020
@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 Jun 25, 2020
@lafriks
Copy link
Member

lafriks commented Jun 26, 2020

🚀

@lafriks lafriks merged commit 0ea4795 into go-gitea:master Jun 26, 2020
@CirnoT CirnoT deleted the internal-icon-av branch June 26, 2020 11:33
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Add org avatar on top of internal repo icon

* add color for arc-green

* use wrapper div to avoid negative margins

* rename class and move div

* move div to icon tmpl

* remove unnecessary margin for lock octicon

* fix label align together with go-gitea#11891

Co-authored-by: techknowlogick <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants