-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Even more repo rework #15005
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
Even more repo rework #15005
Conversation
modules/templates/helper.go
Outdated
@@ -555,7 +555,8 @@ func SVG(icon string, others ...interface{}) template.HTML { | |||
svgStr = heightRe.ReplaceAllString(svgStr, fmt.Sprintf(`height="%d"`, size)) | |||
} | |||
if class != "" { | |||
svgStr = strings.Replace(svgStr, `class="`, fmt.Sprintf(`class="%s `, class), 1) | |||
matches := regexp.MustCompile(`class="(.*?)"`).FindStringSubmatch(svgStr) |
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.
This is personal taste: I don't want to have <svg>
elements generated by svg.js
and helper.go
to have different order of class names. I originally planned to deduplicate the class names (like in svg.js
) but that would add unnecessary costs with no added benefit.
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 should move regexp.MustCompile
out of the function to not compile the regex every function call.
Forgot to test avatar. Found the culprit of that misalignment: #10425 (comment). Fix is coming. |
960b66e
to
548b7d2
Compare
548b7d2
to
7828478
Compare
templates/repo/branch/list.tmpl
Outdated
@@ -9,21 +9,21 @@ | |||
</h4> | |||
|
|||
<div class="ui attached table segment"> | |||
<table class="ui very basic striped fixed table single line"> | |||
<table class="ui very basic striped fixed table single line unstackable table"> |
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.
There are two table
s now.
87c804c
to
74c9c4e
Compare
templates/org/team/teams.tmpl
Outdated
@@ -37,7 +37,7 @@ | |||
{{end}} | |||
</div> | |||
<div class="ui bottom attached header"> | |||
<p class="team-meta">{{.NumMembers}} {{$.i18n.Tr "org.lower_members"}} · {{.NumRepos}} {{$.i18n.Tr "org.lower_repositories"}}</p> | |||
<p class="team-meta">{{.NumMembers}} {{$.i18n.Tr "org.lower_members"}} · {{.NumRepos}} {{$.i18n.Tr "org.lower_repositories"}}</p> |
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.
Width of
will not be consistent across different fonts and I generally despise using whitespace for layout purposes, better to use something like
<span class="mx-2">·</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.
This goes without saying for me, but I was more focused on other stuff. I might revert this change in exchange to better solutions. white-space: pre
exists at other places as well - I really can't see the use of that apart from supporting this type of weird layout technique with breakable spaces - ugh!
margin-left: 0; | ||
margin-right: 0; | ||
&.flex-wrappable { | ||
margin-bottom: 0; |
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.
Why not use the mb-0
helper instead of adding this class? Also mb-1
for .item
.
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.
This would be a helper class like those it replaces: it applies recustively so I don't have to add mb-1
to every single .item
in case I forget one. Didn't you create those classes (forgive me if I'm wrong)?
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.
Yeah I guess that usage is fine. Yes it was me who added the helpers.
4d2c25f
to
e4bd476
Compare
ee03601
to
d906f8d
Compare
Damn, I have too many local branches and did a force push without actually changing the content of the commit... Sorry for wasting CPU power of CI... |
I've cancelled the job, no worries. 🙂 |
Actually I was planning on this as part of the ongoing CSS variable rework. In the final result, arc-green should only consist of variables. |
{{if .IsProtected}} | ||
{{svg "octicon-shield-lock"}} | ||
{{end}} | ||
<a href="{{$.RepoLink}}/src/branch/{{.Name | EscapePound}}">{{.Name}}</a> | ||
<p class="info df ac my-2">{{svg "octicon-git-commit" 16 "mr-2"}}<a href="{{$.RepoLink}}/commit/{{.Commit.ID.String}}">{{ShortSha .Commit.ID.String}}</a> · <span class="commit-message">{{RenderCommitMessage .Commit.CommitMessage $.RepoLink $.Repository.ComposeMetas}}</span> · {{$.i18n.Tr "org.repo_updated"}} {{TimeSince .Commit.Committer.When $.i18n.Lang}}</p> | ||
<p class="info df ac my-2" title="{{$renderedCommitMessage}}"> | ||
{{- svg "octicon-git-commit" 16 "mr-2"}}{{/* p */ -}} |
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.
What does this /* p */
do?
--color-yellow-sha-bg: rgba(251, 189, 8, .1); | ||
--color-yellow-sha-hover: rgba(251, 189, 8, .3); | ||
--color-green-sha-bg: rgba(33, 186, 69, .1); | ||
--color-green-sha-hover: rgba(33, 186, 69, .3); |
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.
- Could you convert to
#rrggbbaa
syntax? Just for consistency basically. - Can we use
error
(red),warning
(orange),info
(blue) andsuccess
(green) states please? - Based on previous point, please remove color names from these variable names, e.g.
--color-red-err-bg
->--color-error-bg
,--color-green-ok-bg
->--color-success-bg
.
} | ||
} | ||
} | ||
|
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.
Can we maybe not use nested syntax to override fomantic? I prefer plain selectors in this case because it's easiert to search exact selector strings in fomantic source and just copy from there. Nested syntax would break this searching.
.octicon-tiny > .svg { | ||
margin-top: -2px; | ||
margin-bottom: -2px; | ||
vertical-align: -2px; | ||
} |
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.
This is only used in two places, suggest to remove the class and use helper classes if possible. Also, the previous font-size
does nothing on SVG.
text-align: center; | ||
.sha.mono { | ||
font-size: 1em !important; | ||
width: calc(9ch + 82px); // 60px + 11px * 2 |
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.
Do we really need explicit width on this? I prefer to avoid 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.
This is necessary since the table is fixed, i. e. the width of the columns are have predefined starting values (can be incremented but not decreased). I basically copied the logic from the commits table to the repo file table for consistency.
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.
So essentially you are stretching the column with this width
? Can we be sure it will not cut off for example if we add more content in the hash label?
Can you maybe convert to min-width
?
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.
So essentially you are stretching the column with this
width
? Can we be sure it will not cut off for example if we add more content in the hash label?
I'm pretty sure it will cut off if more content is added.
Can you maybe convert to
min-width
?
It won't work. If I recall correctly, when the width of a fixed table decreases, only explicit width
(either as percentage in explicit numbers) could have an effect on column widths. No actual content width would be taken in to consideration. It's common practice though and ensures the fastest layout possible for large tables.
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.
To make it clear again: the commits table has always been fixed, and there wasn't any measure taken to ensure that the SHA label would fit (the Fomantic UI classes define the percentage width of the columns, just like the flex-based grid (but does not support that many layout strategies). It basically gives up at lower resolution and wraps the table in three rows - that design may find its use elsewhere, but the UE is awful in this case. I thus use media queries to hide the less-important elements explicitly, but otherwise keep the design same as before (it has the advantage of always getting the overflow-text: ellipsis
right for commit messages embedded in flexboxes, so I also ported that to the repo file table by using a complex table header with a zero-height borderless top row, used just to set the width of the columns - I'm yet to find anything more elegant than that based on what's already there).
Ideally, we could copy GitHub's idea of using a bunch of nested flexboxes (with helper classes based on media queries) given their versatility, but those are pretty hard to tame...
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.
Also, thanks for all the reviews. I've read all of them and have them on my checklist. Have other things to do...
padding-bottom: 3px; | ||
} | ||
display: flex; | ||
height: 21.6px; // 14px + 2.8px * 2 + 1px * 2 |
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 question here, would like to avoid any explicit dimensions.
--color-yellow-sha-bg: rgba(251, 189, 8, .1); | ||
--color-yellow-sha-hover: rgba(251, 189, 8, .3); | ||
--color-green-sha-bg: rgba(33, 186, 69, .1); | ||
--color-green-sha-hover: rgba(33, 186, 69, .3); |
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.
Please copy the remaining vars from _base.less
in here as well, I'd prefer this list to stay in sync, even if colors are the same.
border-color: #393d4a; | ||
color: #dbdbdb; | ||
} | ||
|
||
.repository .ui.attached.message.isSigned.isVerified { | ||
background-color: #394829; | ||
color: var(--color-secondary-dark-6); |
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.
There might be more rules to remove in theme-arc-green.less
for the SHA labels, please verify.
Please resolve the conflicts. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
If no one could pick up this one. I think we have to close it. |
It's a pity that this PR stales for so long time and can not be merged. Thank you, CL-Jeremy (I saw a lot of UI work from you before 👍) Fortunately, a lot of problems has been resolved by some recent PRs separately. I agree it's the time to close this one (if still no new progress) and move on to open new PRs to make UI better. |
oh no @CL-Jeremy it got some conflicts - do you need help resolving them - just ask :) |
OK, let me be the bad guy to close this PR, since it has been stale for quite a long time .... and a lot of problems has been resolved by some recent PRs separately. Feel free to reopen it or pick it as a new one. 🙏 |
The previous rework marks only the beginning of a big rework towards mobile friendliness. The main change is that tables are now unstackable on mobile: no more overly long page scrolling (figure 1).
This PR includes many more stuff I'd like to fix urgently. #14704 was focusing on the tabular menu, but I went on to include language statistics as well as it is related in design concept (finger-scrollable with an auto-hiding scrollbar). Further changes would make that PR impossible to be reviewed.
This PR includes but is not limited to the following changes as of now (As this is just a draft, I'd like to let it settle a bit and summarize the changes more thoroughly during that time):
Repo home
index.js
) for newly created labels, and correctly shows a different red color (figure 3; I invented the colors - need to be formalized) for invalid labels (previously it was impossible to tell which labels are actually invalid, since all subordinate labels to a semantic multiple search bar in error state are dyed red, and that red theme is exactly the same red as the defined accent red), and valid labels get better colors with both arc-green and gitea themesRepo commit list (history)
vertical-align: middle
, the button is shorter in height, uses octicon, and aligns properly with other commits without such a button (think of floating layout - right margin is aligned with surrounding texts, which used to extend only to its left margin, wasting some space) - most of these changes are reflected in the same wrapper on file list table header (ported over to save some layout effort, even usingdisplay: table-cell
for neighboring elements)Repo branches
Git graph (new)
Global
12px14px icon instead of previous 16px, which was way too big and made signed labels much taller than unsigned labels, creating many problemsmin-width: max-content
(expect better compatibility thanfix-content
)Bug (
partiallyfixed): overflowing file list table header (now hard truncated without proper ellipsis - detail expansion button is hidden when commit message is too long)Suggestions and comments are highly appreciated as always.
Figure 1:

Figure 2:

Figure 3:

close #15283