Show all contributors in the AboutDialog #7666
Conversation
| } | ||
| $.ajax({ | ||
| url: url, | ||
| async: false, |
There was a problem hiding this comment.
Is there a better method than using async: false?
There was a problem hiding this comment.
I am thinking that maybe we could just get the total pages and then use Async.doInParallel, using an array of pages numbers as the first argument and loadContributorsPage(page) (that returns the ajax/getJSON) as the second parameter.
Another solution is to implement it similar to how Async.doInParallel is done.
There was a problem hiding this comment.
But there's no easy way for getting the total pages, right? The only way I know would be to load the first page synchronouzly and then read the header. It wouldn't improve anything.
There was a problem hiding this comment.
You can do a head ajax request and read the content before doing anything else:
$.ajax({type: "HEAD", url, url}).done(function (data) { ...do the rest... })
|
@TomMalbran would you mind finishing the review on this one? thx |
| @@ -14,7 +14,7 @@ | |||
| "twitter_url" : "https://twitter.com/brackets", | |||
| "troubleshoot_url" : "https://github.com/adobe/brackets/wiki/Troubleshooting#wiki-livedev", | |||
| "twitter_name" : "@brackets", | |||
| "contributors_url" : "https://api.github.com/repos/adobe/brackets/contributors?per_page=300", | |||
| "contributors_url" : "https://api.github.com/repos/adobe/brackets/contributors", | |||
There was a problem hiding this comment.
What do you think about using https://api.github.com/repos/adobe/brackets/contributors?per_page=100&page={0}?
There was a problem hiding this comment.
Jup, that's better.
| var $dlg = $(".about-dialog.instance"), | ||
| $contributors = $dlg.find(".about-contributors"), | ||
| $spinner = $dlg.find(".spinner"), | ||
| allContributors = [], |
There was a problem hiding this comment.
This isn't needed anymore. Same with data 2 lines down
There was a problem hiding this comment.
Good catch. It's a pity our JSLint doesn't catch them lines automatically.
There was a problem hiding this comment.
A newer version of JSLint would catch it. Even after we move to JSHint, we would be able to catch it. But not now that we have a really old version of JSLint.
|
@SAplayer This looks great, just a few more minor things, and we are probably done. |
|
@TomMalbran Ready for another (hopefully the last) review. |
| }) | ||
| .fail(function () { | ||
| if (contributors.length) { // we weren't able to fetch this page, but previous fetches were successful | ||
| deferred.resolve(contributors); |
There was a problem hiding this comment.
Should we also show some kind of error after the loaded contributors. We would show the pictures that we get and a line saying something like And more (but we're having trouble loading that data right now).
If we add that text to the contributors template we need to add a new line at the bottom like: {{#error}}<p>{{Strings.ABOUT_MORE_CONTRIBUTORS}}</p>{{/error}}, when rendering the template pass an object like: {contributors: allContributors, error: error, Strings: Strings}, and change in the template {{#.}} and {{/.}} with {{#contributors}} and {{/contributors}}.
There was a problem hiding this comment.
I don't believe we need this (at least for now), it shouldn't be a common case.
We are loading 3 pages (yes, we got > 200 contributors now), which is done in less than a second.
The only things that could cause such an issue is a very bad internet connection or a fail in the GH API. While the second one is very unlikely, the first one probably won't happen within a second.
There was a problem hiding this comment.
Yes, this is a really uncommon case. I guess we can keep it like that.
|
@SAplayer Just one more idea to add to the dialog. |
| contributorsUrl = brackets.config.contributors_url, | ||
| page; | ||
|
|
||
| if (contributorsUrl.indexOf("{1}") !== -1) { // pagination enabled |
There was a problem hiding this comment.
I wouldn't do it. This request changes the URL and the code should rely on the paging enabled. If it is not, then it should break (and it's OK). We should probably add a testcase for that.
There was a problem hiding this comment.
I did this code because I don't know which URL is used by Edge Code, or even if they use this feature at all.
But you're right, we can probably just remove it.
There was a problem hiding this comment.
I actually added the url to the config file since we were adding all the urls there, but not because it might be changed by Edge Code.
Saying that, I don't know if Edge Code actually changes it. So we might need to ask the team about it.
There was a problem hiding this comment.
I just looked at it again and saw that the current implemention is not relying on pagination enabled, but it would be if we removed this particular line.
I guess it's better the way it is, so I won't change it.
|
Notice I won't be able to do any changes on this until Thurday. |
|
It looks like the team is getting ready to branch the release branch for Release 39, and this issue is not marked for Release 39. So I guess it might be ok to move it to Release 40. |
|
It's fine with me now. |
|
I think it looks good to me too. I'll give it a final review and test and merge it. |
|
Code looks and works great. Thanks for fixing this. Merging |
Show all contributors in the AboutDialog
This is the second fix for #7614, which is respecting the pagination of the GitHub API correctly.