-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Contributors List on the About Dialog #2934
Changes from 5 commits
9e204b9
ff36310
e36617e
c3eaea7
0600c7f
d82c112
0365cb0
0fca970
5949dd4
0f5c15c
fa22529
ea93581
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 |
---|---|---|
|
@@ -23,7 +23,7 @@ | |
|
||
|
||
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
/*global define, $, brackets, window, Mustache */ | ||
/*global define, $, brackets, window, PathUtils, Mustache */ | ||
|
||
define(function (require, exports, module) { | ||
"use strict"; | ||
|
@@ -38,11 +38,73 @@ define(function (require, exports, module) { | |
UpdateNotification = require("utils/UpdateNotification"), | ||
FileUtils = require("file/FileUtils"), | ||
NativeApp = require("utils/NativeApp"), | ||
PreferencesManager = require("preferences/PreferencesManager"), | ||
StringUtils = require("utils/StringUtils"), | ||
AboutDialogTemplate = require("text!htmlContent/about-dialog.html"); | ||
AboutDialogTemplate = require("text!htmlContent/about-dialog.html"), | ||
ContributorsTemplate = require("text!htmlContent/contributors-list.html"); | ||
|
||
var buildInfo; | ||
|
||
|
||
// PreferenceStorage | ||
var _prefs = PreferencesManager.getPreferenceStorage(module.id); | ||
|
||
// Last time the contributorsInfo was fetched | ||
var _lastContributorsFetchTime = _prefs.getValue("lastContributorsFetchTime"); | ||
|
||
|
||
/** | ||
* @private | ||
* Gets a data structure that has the information for all the contributors of Brackets. | ||
* The information is fetched from brackets.config.contributors_url using the github API. | ||
* If more than 2 weeks have passed since the last fetch, or if cached data can't be found, | ||
* the data is fetched again. | ||
* @return {$.Promise} jQuery Promise object that is resolved or rejected after the information is fetched. | ||
*/ | ||
function _getContributorsInformation() { | ||
var result = new $.Deferred(); | ||
var fetchData = false; | ||
var data; | ||
|
||
// If we don't have data saved in prefs, fetch | ||
data = _prefs.getValue("contributorsInfo"); | ||
if (!data) { | ||
fetchData = true; | ||
} | ||
|
||
// If more than 2 weeks have passed since our last fetch, fetch again | ||
if ((new Date()).getTime() > _lastContributorsFetchTime + (14 * 24 * 60 * 60 * 24)) { | ||
fetchData = true; | ||
} | ||
|
||
if (fetchData) { | ||
$.getJSON(brackets.config.contributors_url + "?callback=?", function (contributorsInfo) { | ||
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. Because GitHub uses the Access-Control-Allow-Origin: *, would it be better to use a regular $.get(brackets.config.contributors_url, function (contributorsInfo) { 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 don't see any difference in how getJSON would be any different to get, since actually getJSON is used with a dataType of JSONP. But you might know better and I could even simply make it a 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. It's actually not I put together a quick test at http://jsperf.com/get-vs-getjson-vs-ajax which shows the performance hit that |
||
data = []; | ||
|
||
// Save only the required data for the template | ||
$.each(contributorsInfo.data, function (index, element) { | ||
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 use forEach() instead of $.each() |
||
data.push({ | ||
GITHUB_URL : element.html_url, | ||
AVATAR_URL : element.avatar_url, | ||
NAME : element.login | ||
}); | ||
}); | ||
_lastContributorsFetchTime = (new Date()).getTime(); | ||
_prefs.setValue("lastContributorsFetchTime", _lastContributorsFetchTime); | ||
_prefs.setValue("contributorsInfo", data); | ||
|
||
result.resolve(data); | ||
|
||
}).error(function () { | ||
result.reject(); | ||
}); | ||
} else { | ||
result.resolve(data); | ||
} | ||
|
||
return result.promise(); | ||
} | ||
|
||
|
||
function _handleCheckForUpdates() { | ||
UpdateNotification.checkForUpdate(true); | ||
} | ||
|
@@ -69,7 +131,39 @@ define(function (require, exports, module) { | |
APP_NAME_ABOUT_BOX : brackets.config.app_name_about, | ||
BUILD_INFO : buildInfo || "" | ||
}, Strings); | ||
|
||
Dialogs.showModalDialogUsingTemplate(Mustache.render(AboutDialogTemplate, templateVars)); | ||
|
||
// Get all the project contributors and add them to the dialog | ||
_getContributorsInformation().done(function (contributorsInfo) { | ||
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'm not sure why this is, but there seems to be a long pause before the About dialog appears now (compared to master). I don't know why that should be, because the fetch of the contributor info is asynchronous, so it shouldn't actually block the dialog from appearing. Are you seeing that at all? If so, any ideas what could be happening? 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. Long pause seems to be gone. |
||
|
||
// Populate the contributors data | ||
var $dlg = $(".about-dialog.instance"); | ||
var $contributors = $dlg.find(".about-contributors"); | ||
|
||
templateVars = $.extend({CONTRIBUTORS: contributorsInfo}, Strings); | ||
$contributors.html(Mustache.render(ContributorsTemplate, templateVars)); | ||
|
||
// This is used to create an opacity transition when each image is loaded | ||
$dlg.find("img").one("load", function () { | ||
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 should be |
||
$(this).css("opacity", 1); | ||
}).each(function () { | ||
if (this.complete) { | ||
$(this).load(); | ||
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'm not clear on what this does. jQuery has two 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. Yes, it should be a |
||
} | ||
}); | ||
|
||
$dlg.on("click", "img", function (e) { | ||
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. Similarly, this should be |
||
var url = $(e.target).data("url"); | ||
|
||
if (url) { | ||
// Make sure the URL has a domain that we know about | ||
if (/(github\.com)$/i.test(PathUtils.parseUrl(url).hostname)) { | ||
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. Not entirely sure under what scenario a malicious website could end up in here, but if the goal is to guard against that, the regexp should be something like 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 I just remove this then? |
||
NativeApp.openURLInDefaultBrowser(url); | ||
} | ||
} | ||
}); | ||
}); | ||
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. Yup, there should be a |
||
} | ||
|
||
// Read "build number" SHAs off disk immediately at APP_READY, instead | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{{#CONTRIBUTORS}} | ||
<img class="clickable-link" data-url="{{GITHUB_URL}}" src="{{AVATAR_URL}}" title="{{NAME}}" alt="{{NAME}}" width="30" height="30" /> | ||
{{/CONTRIBUTORS}} |
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.
This part of the comment is now out of date.