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

Show all contributors in the AboutDialog #7666

Merged
merged 6 commits into from
May 10, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/brackets.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -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?per_page={0}&page={1}",
"extension_listing_url" : "",
"extension_registry" : "https://s3.amazonaws.com/extend.brackets/registry.json",
"extension_url" : "https://s3.amazonaws.com/extend.brackets/{0}/{0}-{1}.zip",
Expand Down
2 changes: 1 addition & 1 deletion src/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,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?per_page={0}&page={1}",
"extension_listing_url": "",
"extension_registry": "https://s3.amazonaws.com/extend.brackets/registry.json",
"extension_url": "https://s3.amazonaws.com/extend.brackets/{0}/{0}-{1}.zip",
Expand Down
108 changes: 75 additions & 33 deletions src/help/HelpCommandHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,22 @@ define(function (require, exports, module) {
"use strict";

var AppInit = require("utils/AppInit"),
Global = require("utils/Global"),
BuildInfoUtils = require("utils/BuildInfoUtils"),
Commands = require("command/Commands"),
CommandManager = require("command/CommandManager"),
Commands = require("command/Commands"),
Dialogs = require("widgets/Dialogs"),
Strings = require("strings"),
UpdateNotification = require("utils/UpdateNotification"),
FileUtils = require("file/FileUtils"),
Global = require("utils/Global"),
NativeApp = require("utils/NativeApp"),
Strings = require("strings"),
StringUtils = require("utils/StringUtils"),
UpdateNotification = require("utils/UpdateNotification"),
AboutDialogTemplate = require("text!htmlContent/about-dialog.html"),
ContributorsTemplate = require("text!htmlContent/contributors-list.html");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some alphabetical reordering here, nothing special


/** @const This is the thirdparty API's (GitHub) maximum contributors per page limit */
var CONTRIBUTORS_PER_PAGE = 100;

var buildInfo;


Expand Down Expand Up @@ -76,39 +79,78 @@ define(function (require, exports, module) {
Dialogs.showModalDialogUsingTemplate(Mustache.render(AboutDialogTemplate, templateVars));

// Get containers
var $dlg = $(".about-dialog.instance"),
$contributors = $dlg.find(".about-contributors"),
$spinner = $dlg.find(".spinner");
var $dlg = $(".about-dialog.instance"),
$contributors = $dlg.find(".about-contributors"),
$spinner = $dlg.find(".spinner"),
contributorsUrl = brackets.config.contributors_url,
page;

if (contributorsUrl.indexOf("{1}") !== -1) { // pagination enabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

page = 1;
}

$spinner.addClass("spin");

// Get all the project contributors and add them to the dialog
$.getJSON(brackets.config.contributors_url).done(function (contributorsInfo) {

// Populate the contributors data
var totalContributors = contributorsInfo.length;
var contributorsCount = 0;

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

// 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");
}
function loadContributors(rawUrl, page, contributors, deferred) {
deferred = deferred || new $.Deferred();
contributors = contributors || [];
var url = StringUtils.format(rawUrl, CONTRIBUTORS_PER_PAGE, page);

$.ajax({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just use $.getJSON(url)? It was working good before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used $.ajax() to disable caching.

url: url,
dataType: "json",
cache: false
})
.done(function (response) {
contributors = contributors.concat(response || []);
if (page && response.length === CONTRIBUTORS_PER_PAGE) {
loadContributors(rawUrl, page + 1, contributors, deferred);
} else {
deferred.resolve(contributors);
}
})
.fail(function () {
if (contributors.length) { // we weren't able to fetch this page, but previous fetches were successful
deferred.resolve(contributors);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a really uncommon case. I guess we can keep it like that.

} else {
deferred.reject();
}
});
return deferred.promise();
}

loadContributors(contributorsUrl, page) // Load the contributors
.done(function (allContributors) {
// Populate the contributors data
var totalContributors = allContributors.length,
contributorsCount = 0;

allContributors.forEach(function (contributor) {
// remove any UrlParams delivered via the GitHub API
contributor.avatar_url = contributor.avatar_url.split("?")[0];
});

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

// 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));
});
}).fail(function () {
$spinner.removeClass("spin");
$contributors.html(Mustache.render("<p class='dialog-message'>{{ABOUT_TEXT_LINE6}}</p>", Strings));
});
}

// Read "build number" SHAs off disk immediately at APP_READY, instead
Expand Down
2 changes: 1 addition & 1 deletion src/htmlContent/contributors-list.html
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{{#.}}<a href="{{html_url}}" title="{{login}} - {{html_url}}"><img src="{{avatar_url}}" alt="{{login}}" width="30" height="30" /></a>{{/.}}
{{#.}}<a href="{{html_url}}" title="{{login}} - {{html_url}}"><img src="{{avatar_url}}?size=30" alt="{{login}}" width="30" height="30" /></a>{{/.}}
11 changes: 4 additions & 7 deletions src/styles/brackets_patterns_override.less
Original file line number Diff line number Diff line change
Expand Up @@ -988,13 +988,10 @@ a[href^="http"] {
a {
text-decoration: none;
}
}
.about-contributors img {
opacity: 0;
-webkit-transition: opacity 1s;
-moz-transition: opacity 1s;
-o-transition: opacity 1s;
transition: opacity 1s;
img {
opacity: 0;
transition: opacity 1s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are fixing this, we can remove all the prefixed versions of transition: http://caniuse.com/#search=transition

}
}
}

Expand Down