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

Added contributors widget #1098

Merged
merged 3 commits into from Jul 7, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Added contributors widget

  • Loading branch information...
sake92 committed Jul 3, 2018
commit 99ae5a6bd710f66feb6903fcf965f003b6a294a6
@@ -20,6 +20,11 @@
<a href="{{page.next-page}}.html"><strong>next</strong> &rarr;</a>
{% endif %}
</div>

<div class="content-contributors">
<h3>Contributors to this page:</h3>
<div id="contributors" class="contributors-container"></div>
</div>
</div>
</div>

@@ -51,6 +51,29 @@
}
}

.content-contributors {
.contributors-container {
display: flex;
flex-wrap: wrap;
align-items: center;
div {
margin: 5px;
a {
vertical-align: middle;
padding: 3px;
text-decoration: none;
}
img {
vertical-align: middle;
width: 35px;
height: 35px;
margin-bottom: 0;
border-radius: 7px;
}
}
}
}

.content-primary {
.documentation,
.tools {
@@ -521,4 +521,58 @@ $(document).ready(function(){
$("html, body").animate({ scrollTop: 0 }, 600);
return false;
});
});
});

//Contributors widget
// see https://stackoverflow.com/a/19200303/4496364
$(document).ready(function () {
let githubApiUrl = 'https://api.github.com/repos/scala/docs.scala-lang/commits';
// transform "/tour/basics.html" to "_ba/tour/basics.md";
let thisPageUrl = window.location.pathname;
thisPageUrl = thisPageUrl.substring(1, thisPageUrl.lastIndexOf('.'));
thisPageUrl = '_' + thisPageUrl + '.md';
// e.g. https://api.github.com/repos/scala/docs.scala-lang/commits?path=README
let url = githubApiUrl + '?path=' + thisPageUrl;
$.get(url, function (data, status) {

let contributorsUnique = [];
let res = data.forEach(commit => {
// add if not already in array
let addedToList = contributorsUnique.find(c => {
let matches = c.authorName == commit.commit.committer.name;
if (!matches && commit.author) {
matches = c.authorName == commit.author.login;
}
return matches;
});

if (!addedToList) {
// first set fallback properties
let authorName = commit.commit.committer.name;
let authorLink = 'mailto:' + commit.commit.committer.email;

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jul 5, 2018

Contributor

Is it really a good idea to have commiter mailto links in a href? This feels unconfortable to me.

This comment has been minimized.

Copy link
@sake92

sake92 Jul 5, 2018

Author Contributor

I agree that's sensitive information. We can put just "#" then?
Although, users should be aware that their public commits must have an email always.
You can have multiple emails associated with Github account.

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jul 5, 2018

Contributor

Alternatively, a link to the github account? Is that available at this point?

This comment has been minimized.

Copy link
@sake92

sake92 Jul 5, 2018

Author Contributor

It does link to github account. Email is just a fallback for users which deleted their Github account (author field is null). See example for user Ghildiyal here: https://api.github.com/repos/scala/docs.scala-lang/commits?path=_tour/tour-of-scala.md


Unrelated to this, one issue I just got is Github's rate limiting (60 requests per hour)... 😒
IDK if there is a server app for Scala, so we could use OAuth?

If not, I'll try to use conditional requests

This comment has been minimized.

Copy link
@jvican

jvican Jul 5, 2018

Member

@SethTisue Do we have a github token that Sakib can use for the authentication?

@sake92 In the meanwhile, implementing conditional requests make sense. Even if we have authentication, we'll have a limit of 5000 requests per hour. I suspect we'll also hit that.

Ideally, we would like to persist the author information in the same repo. Is there a way to do that?

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jul 5, 2018

Contributor

Ah, now I see! If it's available, you take the author link, if not, the committer email of the commit - that's probably a much rarer problem.

If we do take an email, that should probably be the author data, not the committer data (commit.commit.author.name and commit.commit.author.email).

That said, I still don't think we should be providing mailto links. That's what I want in roughly 0% of situations, and I suspect that's the same for others.

My personal preferences, in decreasing order of preferentiality in case we only have an email address:

  • A placeholder link: an a element with no href attribute
  • A fragment link: An a element with a # href
  • No a element at all

@jvican wdyt?

This comment has been minimized.

Copy link
@jvican

jvican Jul 5, 2018

Member

sounds good to me!

This comment has been minimized.

Copy link
@sake92

sake92 Jul 5, 2018

Author Contributor

@martijnhoekstra Fixed both issues:

  • removed mailto:, now it's just <a>authorName</a>
  • using commit.commit.author.name as fallback instead of commit.commit.committer.name

@jvican Regarding caching, I got tricked by Chrome devtools... xD I had Disable cache turned on, so it didn't do caching. Browser does a great job for us, sends headers automatically (you can check rate limiting stays same in X-RateLimit-Remaining header).

This comment has been minimized.

Copy link
@jvican

jvican Jul 6, 2018

Member

I'm a website newbie, so I have no idea how these headers work, but do they work across different clients or is the caching happening only per browser? I'm asking this because I would suspect we overpass the rate limit if we get, say, 5001 users visiting the docs.

Wouldn't there be an option to do this on the backend side instead of the frontend side? A tool we could use to persist this information in the same markdown, or even in the templates generated by Jekyll?

This comment has been minimized.

Copy link
@sake92

sake92 Jul 6, 2018

Author Contributor

This is only browser cache, local. Github says that 60 req/hour is per IP adress, so every user would be able to view 60 pages per hour. Hmm.. we could probably live with this?

If we want to make use of OAuth we must have a backend and a database to store these results.
And then we'd just use jQuery or whatever to fetch these...

let authorImageLink = 'https://github.com/identicons/' + commit.commit.committer.name + '.png';
// if author present, fill these preferably
if (commit.author) {
authorName = commit.author.login;
authorLink = commit.author.html_url;
authorImageLink = commit.author.avatar_url;
}
contributorsUnique.push({
'authorName': authorName,
'authorLink': authorLink,
'authorImageLink': authorImageLink
});
}
});

let contributorsHtml = '';
contributorsUnique.forEach(contributor => {
let contributorHtml = '<div>';
contributorHtml += '<img src="' + contributor.authorImageLink + '">';
contributorHtml += '<a href="' + contributor.authorLink + '">' + contributor.authorName + '</a>';
contributorHtml += '</div>';
contributorsHtml += contributorHtml;
});
$('#contributors').html(contributorsHtml);
});
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.