Add "n commits" link to contributors in contributors graph page #29429

sahinakkaya · 2024-02-26T16:29:15Z

routers/web/repo/contributors.go

web_src/js/components/RepoContributors.vue

Co-authored-by: silverwind <me@silverwind.io>

silverwind · 2024-02-26T17:19:25Z

web_src/js/components/RepoContributors.vue

@@ -88,6 +91,9 @@ export default {
  methods: {
    sortContributors() {
      const contributors = this.filterContributorWeeksByDateRange();
+      const min = dayjs(this.xAxisMin).format('YYYY-MM-DD');
+      const max = dayjs(this.xAxisMax).format('YYYY-MM-DD');
+      this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min}, before:${max}, author:`;


Do we need to encodeURIComponent(this.repoBranch)? Try with a branch name that contains a space character.

Yup.

Actually this.repoBranch might need to use path segment escape.

The q parameter also needs to be escaped (encodeURIComponent)

Actually this.repoBranch might need to use path segment escape.

There is no path escape function in JS, I think encodeURIComponent is suitable for alle cases, path or search params.

I guess "org/repo/branch/feature%2Fbranch" doesn't work.

Yes, you can not pass / to encodeURIComponent if it's part of the path, it's meant for individual path segments or search param values.

delvh · 2024-02-26T17:42:39Z

web_src/js/components/RepoContributors.vue

@@ -88,6 +91,9 @@ export default {
  methods: {
    sortContributors() {
      const contributors = this.filterContributorWeeksByDateRange();
+      const min = dayjs(this.xAxisMin).format('YYYY-MM-DD');
+      const max = dayjs(this.xAxisMax).format('YYYY-MM-DD');
+      this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min}, before:${max}, author:`;


Suggested change

this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min}, before:${max}, author:`;

this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min},before:${max},author:`;

also, it looks really weird to construct the search query like this.
Wouldn't it make more sense to have ?after=…&before=…&author=…?
And is all that functionality already supported?
I would have guessed that you need to add it yourself…

Ideally we should use URLSearchParams to construct search params:

String(new URLSearchParams({foo: "bar", baz: "a b"})) // 'foo=bar&baz=a+b'

Wouldn't it make more sense to have ?after=…&before=…&author=…?

yeah, it would. But I can't do that way because it is not something I decided. Gitea currently works the other way.

And is all that functionality already supported?

Yes. And it won't work if I accept your code suggestion. The spaces are important. As I said, this is how it works.

If it's existing API, I guess this weird format is okay, otherwise it would be a breaking change. Still we should ensure the individual paramters and path segments are encoded correctly.

delvh · 2024-02-26T17:43:25Z

web_src/js/components/RepoContributors.vue

@@ -384,7 +390,7 @@ export default {
              {{ contributor.name }}
            </h4>
            <p class="gt-font-12 gt-df gt-gap-2">
-              <strong v-if="contributor.total_commits">{{ contributor.total_commits.toLocaleString() }} {{ locale.contributionType.commits }}</strong>
+              <strong v-if="contributor.total_commits"><a :href="searchQuery + contributor.email">{{ contributor.total_commits.toLocaleString() }} {{ locale.contributionType.commits }}</a></strong>


What about users having multiple emails?
For those, only the primary email will be counted if I see that correctly…

All email addresses of a user are mapped to their primary email address in backend. See:

gitea/services/repository/contributors_graph.go

Lines 250 to 254 in 324626a

if u != nil {

// update userEmail with user's primary email address so

// that different mail addresses will linked to same account

userEmail = u.GetEmail()

}

If I don't do that, we will see same user multiple times in the contributor graphs page (for multiple emails). And problem you mentioned will be fixed.

But I would prefer it this way as it looks cleaner. I guess it doesn't have to be perfect all the times. If you really want it to be perfect, you may modify the backend code to include commits of a user with their other email addresses when queried (maybe it already works this way, I didn't check)

sahinakkaya · 2024-02-26T18:40:21Z

Everyone reviewing this PR, please experiment with the following search box before your reviews:

It seems that most of the people aren't aware of the functionality this PR makes use of.

yp05327 · 2024-02-27T06:48:36Z

The design now:

Tried to add muted class to the link:

But it seems that it is not the best resolution, as the color of hover is same to the username's color.
Then there are too many green here:

Maybe this is better?

sahinakkaya · 2024-02-27T10:56:29Z

Yeah, it's a good change @yp05327. Feel free to open a PR in my fork and I will accept it.

silverwind · 2024-02-27T17:58:58Z

Tried to add muted class to the link

This is how GH has it and I think I'm fine with it. Could also mute both links or even use text grey on the second line.

Then there are too many green here

#29283 will make links blue soon on dark theme.

yp05327 · 2024-03-01T00:29:34Z

Yeah, it's a good change @yp05327. Feel free to open a PR in my fork and I will accept it.

Sorry, I'm afraid I can't finish this, as I'm not good at CSS, so the screenshot is made by PPT. 😭

#29283 will make links blue soon on dark theme.

Then the user name and the x commits (hover) will be blue, and too many blue?
Can we add a new class that hover with not change the color and only add underbars?
Then this place will look more clearly with different colors.

silverwind · 2024-03-01T01:19:50Z

Can we add a new class that hover with not change the color and only add underbars?

Yes, in fact I made that exact class in another open PR, we can copy the CSS:

https://github.com/go-gitea/gitea/pull/29344/files#diff-bf3a284bc2ea51adf0333a1ece388b60b4aff10e3ec715a889c6fed44450f2d3,

silverwind · 2024-03-16T16:25:37Z

Can we add a new class that hover with not change the color and only add underbars?

Yes, in fact I made that exact class in another open PR, we can copy the CSS:

https://github.com/go-gitea/gitea/pull/29344/files#diff-bf3a284bc2ea51adf0333a1ece388b60b4aff10e3ec715a889c6fed44450f2d3,

#29847 will add this support for a.suppressed.

Extract from #29344. With this class it's possible to have links that don't color on hover. It will be useful for #29429.

Extract from go-gitea/gitea#29344. With this class it's possible to have links that don't color on hover. It will be useful for go-gitea/gitea#29429. (cherry picked from commit ffeaf2d0bd6c99c486aa7869779bb9ceb0aedad6)

wxiaoguang · 2024-11-10T04:13:58Z

Stale for long time?

Add "n commits" link to contributors in contributors graph page

071184c

pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 26, 2024

github-actions bot added the modifies/frontend label Feb 26, 2024

wxiaoguang reviewed Feb 26, 2024

View reviewed changes

routers/web/repo/contributors.go Outdated Show resolved Hide resolved

Make link text strong

dc8533f

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 26, 2024

Apply suggestions from code review

f20cd38

wxiaoguang approved these changes Feb 26, 2024

View reviewed changes

silverwind reviewed Feb 26, 2024

View reviewed changes

web_src/js/components/RepoContributors.vue Outdated Show resolved Hide resolved

sahinakkaya and others added 2 commits February 26, 2024 19:54

Update web_src/js/components/RepoContributors.vue

abdb275

Co-authored-by: silverwind <me@silverwind.io>

Import dayjs

d26364f

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 Feb 26, 2024

sahinakkaya requested a review from silverwind February 26, 2024 17:11

silverwind reviewed Feb 26, 2024

View reviewed changes

delvh reviewed Feb 26, 2024

View reviewed changes

wxiaoguang self-requested a review February 26, 2024 17:53

GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 26, 2024

silverwind mentioned this pull request Mar 16, 2024

add .suppressed link class #29847

Merged

silverwind added a commit that referenced this pull request Mar 16, 2024

add .suppressed link class (#29847)

ffeaf2d

Extract from #29344. With this class it's possible to have links that don't color on hover. It will be useful for #29429.

silverwind removed the modifies/frontend label Mar 31, 2024

wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Nov 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "n commits" link to contributors in contributors graph page #29429

Add "n commits" link to contributors in contributors graph page #29429

sahinakkaya commented Feb 26, 2024

silverwind Feb 26, 2024

wxiaoguang Feb 26, 2024

silverwind Feb 26, 2024 •

edited

Loading

wxiaoguang Feb 26, 2024

silverwind Feb 26, 2024 •

edited

Loading

delvh Feb 26, 2024

silverwind Feb 26, 2024 •

edited

Loading

sahinakkaya Feb 26, 2024

silverwind Feb 26, 2024

delvh Feb 26, 2024

sahinakkaya Feb 26, 2024

sahinakkaya commented Feb 26, 2024

yp05327 commented Feb 27, 2024

sahinakkaya commented Feb 27, 2024

silverwind commented Feb 27, 2024 •

edited

Loading

yp05327 commented Mar 1, 2024

silverwind commented Mar 1, 2024

silverwind commented Mar 16, 2024

wxiaoguang commented Nov 10, 2024

	this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min}, before:${max}, author:`;
	this.searchQuery = `${this.repoLink}/commits/branch/${this.repoBranch}/search?q=after:${min},before:${max},author:`;

	if u != nil {
	// update userEmail with user's primary email address so
	// that different mail addresses will linked to same account
	userEmail = u.GetEmail()
	}

Add "n commits" link to contributors in contributors graph page #29429

Are you sure you want to change the base?

Add "n commits" link to contributors in contributors graph page #29429

Conversation

sahinakkaya commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahinakkaya commented Feb 26, 2024

yp05327 commented Feb 27, 2024

sahinakkaya commented Feb 27, 2024

silverwind commented Feb 27, 2024 • edited Loading

yp05327 commented Mar 1, 2024

silverwind commented Mar 1, 2024

silverwind commented Mar 16, 2024

wxiaoguang commented Nov 10, 2024

silverwind Feb 26, 2024 •

edited

Loading

silverwind Feb 26, 2024 •

edited

Loading

silverwind Feb 26, 2024 •

edited

Loading

silverwind commented Feb 27, 2024 •

edited

Loading