Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix AboutDialog to show more than 30 contributors again #7618

Merged
merged 1 commit into from
Apr 25, 2014

Conversation

marcelgerber
Copy link
Contributor

This fixes #7614 and #7615.

I hardcoded it to show 300 contributors at max (because the dialog would get pretty inflated with more), but we can simply increase the limit.

@njx
Copy link

njx commented Apr 24, 2014

Looks like this didn't get assigned a priority in standup. I'm inclined to call it medium priority, even though it's obviously not a functional issue, because we need to make sure we continue to acknowledge our community contributors. Plus the fix is relatively simple :)

@JeffryBooher
Copy link
Contributor

works pretty good...merging

JeffryBooher added a commit that referenced this pull request Apr 25, 2014
Fix AboutDialog to show more than 30 contributors again
@JeffryBooher JeffryBooher merged commit 6884689 into adobe:master Apr 25, 2014
@marcelgerber
Copy link
Contributor Author

@njx For acknowledging the work of contributors, are you ok with showing "just" 300 contributors?
As I said above, I hardcoded this limit to prevent the dialog from getting too big and loading too long, but I wonder what's your opinion on that.

@marcelgerber marcelgerber deleted the about-dialog-contributors branch April 25, 2014 20:37
@njx
Copy link

njx commented Apr 25, 2014

I think if we're lucky enough to get to more than 300 contributors, we can just increase the limit again :) I'm fine with not creating a more general solution right now.

@njx
Copy link

njx commented Apr 25, 2014

(I believe we were at something like 200 contributors the last time I checked, so I think we have awhile before we run into that problem...)

@marcelgerber
Copy link
Contributor Author

Well, I'm a bit confused now. It looks like the max per_page value GitHub allows is 100 (it could be that this limit was there before, but we just didn't notice it), which is making it much more difficult for us to show all contributors.
Try https://api.github.com/repos/adobe/brackets/contributors?per_page=300&page=2

We've got exactly 198 contributors right now. Not bad.

@njx
Copy link

njx commented Apr 25, 2014

So does that mean that the PR that got merged isn't actually showing all the contributors? If so, then I guess we have to bite the bullet and iterate over all the pages.

If I remember the GitHub API (at least for some other cases I've used), there's actually a link in the returned data from the first page that gives you the proper link to follow to get the next page. So we don't have to hard-code the URLs for future pages.

@marcelgerber
Copy link
Contributor Author

Yes, this means basically that this PR is only showing 100 contributors of 198.
I just wrote to the GitHub support asking for the best way, but I don't expect an API change.

And no, there's no link to the next page, but that's not a big problem. Hardcoding shouldn't be a problem.

@njx
Copy link

njx commented Apr 25, 2014

Ah, you're right. But yeah, it's not a big deal to just set the parameter for each page.

@marcelgerber
Copy link
Contributor Author

Ah, I just got an answer from the support. That's what they said:


Sorry this tripped you up. We announced on the blog 1 and via the Developer Program 2 that we'd start enforcing pagination on many resource listings. The max page size is 100 and you'll need to follow pagination links 3 in the response headers to fetch the rest.

There's no way to get those in a single request. We have to enforce pagination to keep the API fast for everyone. I'm sure you understand.

@njx
Copy link

njx commented Apr 25, 2014

Ah, right - the pagination links are in the headers, not in the body of the response:

Link: <https://api.github.com/repositories/2935735/contributors?per_page=100&page=2>; rel="next", <https://api.github.com/repositories/2935735/contributors?per_page=100&page=2>; rel="last"

But there's no reason to use those - we can just use the hard-coded URL and set the page parameter.

@TomMalbran
Copy link
Contributor

We might want it to find the last page and stop requesting data, but the same can be done when the data returned is empty.

@marcelgerber
Copy link
Contributor Author

If you can wait until Sunday, I can put up a PR then. Else, you can try it yourselves.

@TomMalbran I thought about a check if (data.length < 100) lastPage = true, so that the next page wouldn't even be loaded.

@TomMalbran
Copy link
Contributor

That make sense too. Notice that in the case of having n*100 contributors the last request will have length 0, but that might not even be a case to consider separately.

@njx
Copy link

njx commented Apr 25, 2014

If we happen to do one extra GET in that case it won't kill us :)

@marcelgerber
Copy link
Contributor Author

That's a very rare case, and there's no other way to handle it (When we are on the second page with now 200 contributors, we can't know how many there are on the third page).

@TomMalbran
Copy link
Contributor

The point is not that we would do an extra request. That is fine, but to notice that it will have 0 results. But anyway if we are collecting all the data into one array, that won't even matter.

contributors = [];
page = 1;
lastPage = false;

while (!lastPage) {
   data = requestContributors(page);
   contributors += data;
   page++;
   lastPage = data.length < 100;
}

@njx
Copy link

njx commented Apr 25, 2014

Yup. Or we could incrementally update the display as we go along (with the spinner moving down below the previous "page" of results), which might be slightly nicer, but that's probably gravy.

@TomMalbran
Copy link
Contributor

In that case I guess that the second load of contributors could happen after the user scrolls to the bottom of the list. We could show the loader by default, but delay the request.

@marcelgerber
Copy link
Contributor Author

@TomMalbran That would be a very cool solution, but the most effort as well.
Is += working for arrays? I would have used contributors.concat(data), but if your way works, that's even better. Good to know.

@TomMalbran
Copy link
Contributor

Actually, it doesn't work. So you need to use concat... Too much PHP lately

@TuckerWhitehouse
Copy link
Contributor

Hey, just an FYI, the response headers appear to only include the link to the "next" page if there is one. So by using these you could avoid making that extra request.

@marcelgerber
Copy link
Contributor Author

I'd suggest we should first work on a simple and working implementation, which needn't do more than showing all the contributors, and after that we can work on a nicer behavior.

The positive side effect is that there is no hardcoded limit any more.

@marcelgerber
Copy link
Contributor Author

@TuckerWhitehouse I thought about that as well, but it's really only a 1% chance to be in such a situation...

@TuckerWhitehouse
Copy link
Contributor

I was curious how hard it would be to implement this using the headers provided by github so I wrote the the following code to be used in place of https://github.com/adobe/brackets/blob/master/src/help/HelpCommandHandlers.js#L86-L111

Because this uses the headers from github, which only includes a "next" if there is a next page, this will never make an unnecessary request.

(function _getContributors(url, contributorsList) {
    $.getJSON(url).done(function (contributorsInfo, statusText, jqXHR) {
        // Append Paged Contributors
        contributorsList = contributorsList.concat(contributorsInfo);

        // Get Link Header
        var Link = jqXHR.getResponseHeader('Link');
        var hasNext = false;
        Link.split(',').some(function (item) {
            if (item.indexOf('rel="next"') > -1) {
                _getContributors(item.slice(item.indexOf('<')+1, item.indexOf('>')), contributorsList);
                hasNext = true;
                return true;
            }
        });

        // All Pages Loaded
        if (!hasNext) {
            // Populate the contributors data
            var totalContributors = contributorsList.length;
            var contributorsCount = 0;

            $contributors.html(Mustache.render(ContributorsTemplate, contributorsList));

            // This is used to create an opacity transition when each image is loaded
            $contributors.find("img").one("load", function () {
                $(this).css("opacity", 1);

                // Count the contributors loaded and hide the spinner once all are loaded
                contributorsCount++;
                if (contributorsCount >= totalContributors) {
                    $spinner.removeClass("spin");
                }
            }).each(function () {
                if (this.complete) {
                    $(this).trigger("load");
                }
            });
        }
    }).fail(function () {
        $spinner.removeClass("spin");
        $contributors.html(Mustache.render("<p class='dialog-message'>{{ABOUT_TEXT_LINE6}}</p>", Strings));
    });
})(brackets.config.contributors_url, []);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

About dialog isn't showing all committers
6 participants