Added contributors widget #1098
Conversation
There was a problem hiding this comment.
This is awesome, really! ❤️
I'm approving but let's have @SethTisue or @heathermiller have a look at it before we merge.
resources/js/functions.js
Outdated
| // first set fallback properties | ||
| let authorName = commit.commit.committer.name; | ||
| let authorLink = 'mailto:' + commit.commit.committer.email; |
There was a problem hiding this comment.
Is it really a good idea to have commiter mailto links in a href? This feels unconfortable to me.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alternatively, a link to the github account? Is that available at this point?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
aelement with nohrefattribute - A fragment link: An
aelement with a#href - No
aelement at all
@jvican wdyt?
There was a problem hiding this comment.
@martijnhoekstra Fixed both issues:
- removed
mailto:, now it's just<a>authorName</a> - using
commit.commit.author.nameas fallback instead ofcommit.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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
|
@jvican I refactored JS logic to support more pages. The problem is that pages don't follow a consistent pattern. E.g. FAQ is rendered as Also, I didn't put the widget on some general index pages and didn't include it in pages like books, SIPs, downloads, community etc. |
|
I just cloned the repo and check that the layout was correct for small devices. Thank you for this contribution @sake92, it's really good.
|
|
this is really cool ❤️ |
Fixes #44
Following this answer I managed to get this via Github API:
I roughly followed Mozilla's docs page design and their contributors listing.
Some users don't have the "author" field. Probably deleted their profile?
So, the fallback is
commit.commit.committer.namewhich is, I suppose, always present because of Git...Also, if the author isn't present, image icon is generated with https://github.com/identicons. Example here is Ghildiyal user.
Currently, I added this to tour only. IDK did you guys intend to put these on each page?
If so, there could be some problems with getting page URL from Github. IDK if all of them follow same pattern as "tours"..