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

Conversation

Projects
None yet
4 participants
@sake92
Copy link
Contributor

commented Jul 3, 2018

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.name which 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"..

@sake92 sake92 force-pushed the sake92:feature-44 branch from 6a6d384 to 99ae5a6 Jul 3, 2018

@jvican

jvican approved these changes Jul 4, 2018

Copy link
Member

left a comment

This is awesome, really! ❤️

I'm approving but let's have @SethTisue or @heathermiller have a look at it before we merge.

// 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...

@jvican
Copy link
Member

left a comment

Oh! Then this is totally fine! Would it be possible to add this to every tour or page with docs, not just the tour?

@sake92

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2018

@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 /tutorials/FAQ/context-bounds.html but located in _overviews/FAQ and similar. It works on most of the pages (more than 95% where widget is included), it just has problem in those corner cases.
Script just removes the div if the request doesn't succeed (page URL is wrong or something like that).

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.

@jvican

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

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.

screenshot-2018-7-7 introduction scala documentation 1
screenshot-2018-7-7 introduction scala documentation

@jvican

jvican approved these changes Jul 7, 2018

@jvican jvican merged commit 5ac9c20 into scala:master Jul 7, 2018

1 check passed

continuous-integration/drone/pr the build was successful
Details
@SethTisue

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

this is really cool ❤️

@sake92 sake92 referenced this pull request Sep 26, 2018

Closed

republish? #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.