[WIP] Contributors along with their avatars showing up on multi-repository view. #54
Conversation
Rishabh570
changed the title
|
Awesome! Could you share a screenshot?
…On Thu, Oct 4, 2018, 9:36 AM Rishabh Rawat ***@***.***> wrote:
Fixes #29 <#29>
------------------------------
You can view, comment on, or merge this pull request online at:
#54
Commit Summary
- Contributors along with their avatars showing up on multi-repository
view.
File Changes
- *M* dist/community-toolbox.js
<https://github.com/publiclab/community-toolbox/pull/54/files#diff-0>
(9638)
- *M* index.html
<https://github.com/publiclab/community-toolbox/pull/54/files#diff-1>
(3)
- *M* src/community-toolbox.js
<https://github.com/publiclab/community-toolbox/pull/54/files#diff-2>
(87)
Patch Links:
- https://github.com/publiclab/community-toolbox/pull/54.patch
- https://github.com/publiclab/community-toolbox/pull/54.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#54>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AABfJ3K-NuJ7VJCvhj7cy9EQW_BxYucwks5uhg7sgaJpZM4XIH_X>
.
|
|
@jywarren I open this work in progress PR. Once I fix some remaining things, I'll post a screenshot of it :) |
|
Great, very eager to see it!
…On Thu, Oct 4, 2018, 9:44 AM Rishabh Rawat ***@***.***> wrote:
@jywarren <https://github.com/jywarren> I open this *work in progress* PR.
There are some small things to be fixed in this, but most of the work is
done.
What I've done is, fetch all the repos and then store all the *unique*
contributors from all these repos (currently first 30 repos), and store
them in *localStorage*, so that with a page reload we don't hit Github
API redundantly and use *localStorage* instead...
Once I fix some remaining things, I'll post a screenshot of it :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJyYYFt3P6GM6UqHuksvm0w7-xlwxks5uhhCjgaJpZM4XIH_X>
.
|
|
👍 This is one of my top bugs I'd like to see fixed, so thank you in advance!!! |
src/community-toolbox.js
Outdated
| var testRepos = repos.splice(0,2); | ||
| testRepos.map((repo, i) => { | ||
| var repoContributors = fetchRepoContributors(org, repo); | ||
| console.log("total contributors for repo ", i, "are : ", repoContributors ,repoContributors.length, '\n') |
There was a problem hiding this comment.
Here, repoContributors.length is 0 while repoContributors has objects in it...What is the reason for this behaviour??
|
@jywarren I was stuck onto something (implementing promises, using async/await) but none seem to work out...For now I get it to work with the help of
Please take a look :) |
|
take a while to load contributors (approx 20sec)
I think that's OK -- and i'm happy to look at this now but keep in mind
that wherever possible we should try to solve things one function at a time
-- it makes merging PRs much easier and makes for better modularized code.
Thank you though!
…On Fri, Oct 5, 2018 at 7:48 AM Rishabh Rawat ***@***.***> wrote:
@jywarren <https://github.com/jywarren> I was stuck onto something
(implementing promises, using async/await) but none seem to work out...For
now I get it to work with the help of setTimeout function which also
achieves what we want except that opening https://code.publiclab.org for
the first will take a while to load contributors (approx 20sec) but after
that it will never hit Github API for loading contributors again (as long
as localstorage is not cleared out)...
[image: screenshot from 2018-10-05 17-08-40]
<https://user-images.githubusercontent.com/25483260/46533523-87104080-c8c2-11e8-8ec7-51ac6137edea.png>
Please take a look :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJxTvFun5gQcKl4T140vK_ClobgEbks5uh0bjgaJpZM4XIH_X>
.
|
src/community-toolbox.js
Outdated
| }) | ||
|
|
||
|
|
||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
so you did end up using promises? cool!
There was a problem hiding this comment.
Yeah, I used them but things didn't happen the way I thought, so I ended up putting up setTimeout everywhere...(async/await are not allowed by grunt-browserify, it is giving error while grunt build)
src/community-toolbox.js
Outdated
| res.map((item, i) => { | ||
| repos[i] = item.name; | ||
| }); | ||
| // Stores all public lab repos to localstorage (only first 30 repos yet) |
There was a problem hiding this comment.
This could become a new follow-up issue 👍
There was a problem hiding this comment.
Yes, currently fetching publiclab's repos only gives the repos listed on 1st page...
src/community-toolbox.js
Outdated
| }); | ||
| } | ||
| // Stores all unique contributors info to localstorage | ||
| localStorage.setItem('contributors', JSON.stringify(myArr)); |
There was a problem hiding this comment.
Interesting! When would this then get flushed and updated? Could we do once daily? And could we implement this via a flushLocalStorageAllContributors or something... so we could manually refresh it with a 🔄 button or something?
There was a problem hiding this comment.
Currently, the only way to request the API again for contributors is by manually flushing localStorage...but this can easily done, so no worries :)
|
Also, I'd love to have contributors listed with |
|
Updating it 😸 |
|
Maybe we should store the date in the localstorage too, so we know whether
to flush it (if it's older than 1 day, for example?)
…On Fri, Oct 5, 2018 at 1:07 PM Rishabh Rawat ***@***.***> wrote:
Updating it 😸
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ7Y5TCN5Y32ByzCRda9PBqCdscqWks5uh5F9gaJpZM4XIH_X>
.
|
|
Yes, its possible...so should I implement it? |
|
@jywarren I've added the logic for removing contributors data from localStorage after every single day...please take a look :) |
|
This looks great. Did you want to try to push this into your gh-pages branch so we can try it before merging, OR post a screenshot gif using the We should look ahead to tests as well so we don't have to be so tentative :-) Thank you!!!! 👍 |
|
Hi @jywarren , sorry for delay, actually I made gif 3 times but it gets bigger than 10mb everytime and I keep hitting the API limits regularly while testing... I'll create another gif as my API quota gets refilled 😅 |
|
@jywarren Here is the working GIF,
|
|
Ok awesome. Why 20 seconds, precisely? Just waiting for all results to come in? Could we display them as they arrive? It does seem a little long to wait I guess with no feedback. Also, perhaps if we're waiting we should add a spinner icon - fontawesome has a |
|
@jywarren I've given 20seconds to |
|
@jywarren Finally, removed |
|
@jywarren Here it is, it doesn't take 20seconds anymore 😂 Probably 2-3 seconds...
|
|
This is fantastic. Do you think it's ready to merge? |
|
Yes, only if we don't need spinner icon for now... |
|
@jywarren what do you think...do we need a spinner icon? |
|
Let's do spinner in a follow up. Would you mind opening an issue? Merging! I'll publish soon :-))) Great work!!!! |
|
Congrats on merging your first pull request! 🙌🎉⚡️ |
|
Hi, @Rishabh570 -- just another thought here -- does this properly remember to show individual contributors when you go to: after you first go to http://code.publiclab.org#r=all ? |
|
@jywarren Yes, because there are the separate functions for both of the views...I will post a GIF anyways :) |
|
|
Here is GIF showing a separate list of contributors on both the pages...
|
|
Fantastic! I think in testing it i blew through my API limit :-P I'll move to #53 for that though. (including error for ref) GREAT WORK again! |
|
I think the new code uses the limit really fast so the "exhausted" message and localstorage will be much needed. BTW we'd love to thank you on Twitter if you wanted to share your handle on there? 🎉 |
yes, I do believe that...that's why storing the data is the only best option :)
Thats so nice 😸 It is rishabh_570 |
Fixes #29