-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Show all contributors in the AboutDialog #7666
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"); | ||
|
|
||
| /** @const This is the thirdparty API's (GitHub) maximum contributors per page limit */ | ||
| var CONTRIBUTORS_PER_PAGE = 100; | ||
|
|
||
| var buildInfo; | ||
|
|
||
|
|
||
|
|
@@ -76,39 +79,80 @@ 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"), | ||
| allContributors = [], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Can you move this line after contributorsUrl and align the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't needed anymore. Same with data 2 lines down
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. It's a pity our JSLint doesn't catch them lines automatically.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| contributorsUrl = brackets.config.contributors_url, | ||
| data, | ||
| page; | ||
|
|
||
| if (contributorsUrl.indexOf("{1}") !== -1) { // pagination enabled | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am ok either way.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| 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 || $.Deferred(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are mostly using |
||
| contributors = contributors || []; | ||
| var url = rawUrl; | ||
| if (page) { | ||
| url = StringUtils.format(rawUrl, CONTRIBUTORS_PER_PAGE, page); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use an inline if, here. but maybe that is just my preference. Anyway, formating might always work. If there is no |
||
| } | ||
|
|
||
| $.ajax({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we just use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used |
||
| url: url, | ||
| dataType: "json", | ||
| cache: false | ||
| }) | ||
| .done(function (response) { | ||
| var data = response || []; | ||
| contributors = contributors.concat(data); | ||
| if (page && data.length === CONTRIBUTORS_PER_PAGE) { | ||
| loadContributors(rawUrl, page + 1, contributors, deferred); | ||
| } else { | ||
| deferred.resolve(contributors); | ||
| } | ||
| }) | ||
| .fail(function () { | ||
| deferred.reject(contributors); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to reject with the contributors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the current one. I want to resolve the promise if there was at least one page successfully loaded. |
||
| }); | ||
| return deferred; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Just for consistency, even that is it not needed, I would return |
||
| } | ||
|
|
||
| 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 | ||
|
|
||
| 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>{{/.}} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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