Skip to content

Contributors List on the About Dialog #2934

Merged
merged 12 commits into from Mar 1, 2013

6 participants

@TomMalbran

This pull request adds the functionality mentioned in https://trello.com/c/LmtuTQT9 adding every contributor to Brackets as github profile pictures in a grid with a link to their corresponding github profile and a title with their name. The data is also cached for 24 hours to make it load faster.

@njx njx was assigned Feb 22, 2013
@WebsiteDeveloper

seems to work good but i have a suggestion maybe you could add a loading indicator if loading is currently in progress because it looks a bit strange when there is much empty space

@TomMalbran

That empty space is created by the images. The dialog shows nothing until it receive the data from Github and then it populates the dialog with all the profile images and starts loading them. It could be good to add a loading indicator, but it might be as easy or useful just for the images load.

@adrocknaphobia
Adobe Systems Incorporated member

@TomMalbran This looks great. Thanks for taking this on. I have a few requests before we can merge:

  1. Replace "Brackets Contributors:" with "Made with ♥ and JavaScript by:"
  2. The loading delays are an issue. How are your jQuery / animation skills? 😄
    • The dialog should contain at least 3 rows (36 entries) on load (before making the call to github). We can use an empty div with a darkened background, or an image placeholder. This should remove the jarring "pop" when the scollbar comes into view.
    • Once we get the results back from github, loop over the urls and download the images. On load, swap out the empty div/placeholder with the image (preferably with a css transition on the image opacity).
    • Beyond 36, start adding new images 1 by 1 as they load.
  3. Set the cache to 2 weeks (rather than 24 hours).
@TomMalbran

@adrocknaphobia Your welcome and thanks for the review.

  1. Done.
  2. I addressed this a bit different:
    • Instead of a place holder for each image, I made the div have a min-height of 3 rows with a light grey background. I then added all the images to the div at the same time but with an on load event to make the opacity transition. This is mainly because all the information was already parsed in an array and is needed like this to cache the information and restore it easily.
    • I could go with your suggestion, but I think this way has the same effect and with less added code.
    • Right now I see the images load really fast, but I could also add a loading text and remove it once the images are loaded (all or just the first 36).
  3. Done.
@peterflynn peterflynn commented on an outdated diff Feb 26, 2013
src/help/HelpCommandHandlers.js
+ 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) {
+ data = [];
+
+ // Save only the required data for the template
+ $.each(contributorsInfo.data, function (index, element) {
@peterflynn
Adobe Systems Incorporated member
peterflynn added a note Feb 26, 2013

Should use forEach() instead of $.each()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adrocknaphobia
Adobe Systems Incorporated member

@TomMalbran Your solutions looks great.Criteria of the story is met, so once this get reviewed we can merge it!

Thanks!

@TomMalbran

Replaced $.each with forEach as Peter mentioned.

@TuckerWhitehouse TuckerWhitehouse and 1 other commented on an outdated diff Feb 27, 2013
src/help/HelpCommandHandlers.js
+ 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) {

Because GitHub uses the Access-Control-Allow-Origin: *, would it be better to use a regular get as opposed to a getJSON which injects an unnecessary script element into the page?

$.get(brackets.config.contributors_url, function (contributorsInfo) {
@TomMalbran
TomMalbran added a note Feb 27, 2013

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 $.ajax with a dataType: "jsonp" setting if is a better solution.

It's actually not $.get vs $.getJSON but rather the inclusion of ?callback=?. This triggers the dataType: "jsonp" which wraps the data in a callback on the server, and then includes a script element in the page, which is a) unnecessary in this case, and b) is significantly slower.

I put together a quick test at http://jsperf.com/get-vs-getjson-vs-ajax which shows the performance hit that dataType: "jsonp" takes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx
Adobe Systems Incorporated member
njx commented Feb 27, 2013

Hmmm, I'm not seeing the images at all on my Mac--I see the gray background rectangle, and then the outlines of the individual images fade in, but the actual images never load. If I look in the network tab in the inspector, it doesn't even seem to be trying to fetch the images, which seems odd. Any ideas?

Regarding the gray background, I think it would look better if the images were just against the white background of the dialog--it would look weird to have gray in the spaces between them. If we need something to show that it's loading, you could just use the same small spinner we use in Find in Files--maybe just put it after the "Made with <3 and JavaScript" line, and then hide it once the images have loaded. (You could also still have the background div there at the beginning to prop open the space--just make it transparent.)

@njx
Adobe Systems Incorporated member
njx commented Feb 27, 2013

Another thought--is there a reason we have to cache the data? It seems fine to just query the API each time you go into the About dialog--it's not like people will be doing it that often :)--or at least shorten the time between checks to an hour or so (two weeks seems excessive). Also, as a possibly silly point, I could imagine that a new contributor would be excited to see their name show up in the dialog after their first merge, and with the current logic they'd have to wait two weeks before it would show up.

@njx
Adobe Systems Incorporated member
njx commented Feb 27, 2013

I'll try to get to a more detailed code review later today--in the meantime, it would be good if you could try to figure out why the images aren't loading (or give me a pointer as to what I should look at to debug it).

@TomMalbran
  1. Sorry, I broke the code when changing from $.each to forEach, since I didnt noticed that the arguments of the functions are inverted and it should now be element, index or just element. I did tested, but probably I was using an old cached code and not the new one.

  2. Sure, the background color came from Adam's suggestion, but since my implementation was different, I could remove the background color. I'll try to get the the loading spinner, I can now see a long enough time to make it useful. Any idea on what to do if the request fails? (usually when a user is working without internet).

  3. Since everything is cached, I thought it would be a good idea, and it would avoid more ajax requests. But you are right, I think the main reason to check the about menu constantly is to check on which code brackets is running, but fro that you could just close the window before the information loads. And actually I started with 24 hours for that reason, so contributors could see their names fast as soon as possible, but leaving a margin of caching. So between 1 hour and nothing, maybe just no caching is better.

I'll work on that and might be able to push this changes before your next review. (Including the suggested $.get instead of $.getJSON). Thanks for the review!

@njx njx commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
var buildInfo;
-
+
+
+ /**
+ * @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,
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

This part of the comment is now out of date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
+ * @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 data = [];
+
+ $.getJSON(brackets.config.contributors_url + "?callback=?", function (contributorsInfo) {
+ // Save only the required data for the template
+ contributorsInfo.data.forEach(function (contributor) {
+ data.push({
+ GITHUB_URL : contributor.html_url,
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

Minor stylistic nitpick: seems like you could just use normal lowercase keys here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
+ function _getContributorsInformation() {
+ var result = new $.Deferred();
+ var data = [];
+
+ $.getJSON(brackets.config.contributors_url + "?callback=?", function (contributorsInfo) {
+ // Save only the required data for the template
+ contributorsInfo.data.forEach(function (contributor) {
+ data.push({
+ GITHUB_URL : contributor.html_url,
+ AVATAR_URL : contributor.avatar_url,
+ NAME : contributor.login
+ });
+ });
+ result.resolve(data);
+
+ }).error(function () {
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

We've been using the promise form for jQuery AJAX calls in general, so it would be good to use done()/fail() instead of using the passed-in callback and error().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
Dialogs.showModalDialogUsingTemplate(Mustache.render(AboutDialogTemplate, templateVars));
+
+ // Get all the project contributors and add them to the dialog
+ _getContributorsInformation().done(function (contributorsInfo) {
+
+ // Populate the contributors data
+ var $dlg = $(".about-dialog.instance");
+ var $contributors = $dlg.find(".about-contributors");
+ var totalContributors = contributorsInfo.length;
+ var contributorsCount = 0;
+
+ $contributors.html(Mustache.render(ContributorsTemplate, {CONTRIBUTORS: contributorsInfo}));
+
+ // This is used to create an opacity transition when each image is loaded
+ $dlg.find("img").one("load", function () {
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

This should be $contributors.find("img") so it doesn't affect other images in the dialog (like the Brackets icon).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
+ $dlg.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) {
+ $dlg.find(".about-spinner").css("display", "none");
+ }
+ }).each(function () {
+ if (this.complete) {
+ $(this).load();
+ }
+ });
+
+ // Create a link for each contributor image to their github account
+ $dlg.on("click", "img", function (e) {
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

Similarly, this should be $contributors.on(...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx and 1 other commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
+ var contributorsCount = 0;
+
+ $contributors.html(Mustache.render(ContributorsTemplate, {CONTRIBUTORS: contributorsInfo}));
+
+ // This is used to create an opacity transition when each image is loaded
+ $dlg.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) {
+ $dlg.find(".about-spinner").css("display", "none");
+ }
+ }).each(function () {
+ if (this.complete) {
+ $(this).load();
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

I'm not clear on what this does. jQuery has two load() methods, but neither of them seem to be relevant here: one takes a url argument and replaces the content of the current element with the fetched content from the URL; the other binds an event handler to the "load" event.

@TomMalbran
TomMalbran added a note Feb 28, 2013

Yes, it should be a trigger("load") used just to trigger the event for cached images that supposedly don't trigger the load event by themselves. But it has been working with that until now, so I am not sure if it really does what it supposed to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx and 1 other commented on an outdated diff Feb 28, 2013
src/help/HelpCommandHandlers.js
+ contributorsCount++;
+ if (contributorsCount >= totalContributors) {
+ $dlg.find(".about-spinner").css("display", "none");
+ }
+ }).each(function () {
+ if (this.complete) {
+ $(this).load();
+ }
+ });
+
+ // Create a link for each contributor image to their github account
+ $dlg.on("click", "img", function (e) {
+ 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)) {
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

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 /(^|\.)github\.com$/ (so a site like "myfakegithub.com" wouldn't match).

@TomMalbran
TomMalbran added a note Feb 28, 2013

Should I just remove this then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on the diff Feb 28, 2013
src/help/HelpCommandHandlers.js
Dialogs.showModalDialogUsingTemplate(Mustache.render(AboutDialogTemplate, templateVars));
+
+ // Get all the project contributors and add them to the dialog
+ _getContributorsInformation().done(function (contributorsInfo) {
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

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?

@njx
Adobe Systems Incorporated member
njx added a note Mar 1, 2013

Long pause seems to be gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on an outdated diff Feb 28, 2013
src/styles/brackets_patterns_override.less
+ }
+ .about-contributors {
+ min-height: 100px;
+ }
+ .about-contributors img {
+ opacity: 0;
+ -webkit-transition: opacity 1s;
+ -moz-transition: opacity 1s;
+ -o-transition: opacity 1s;
+ transition: opacity 1s;
+ }
+ .about-spinner {
+ display: inline-block;
+ vertical-align: middle;
+ margin-top: -2px;
+ margin-left: 10px;
@njx
Adobe Systems Incorporated member
njx added a note Feb 28, 2013

This feels a little big to me. I think you could just take this margin out--it looks okay to me without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx
Adobe Systems Incorporated member
njx commented Feb 28, 2013

Initial review complete. My only real concern is about the apparent delay before the dialog appears--let me know if you're not seeing that.

I'm going to be out at an offsite tomorrow, but I'll let the team know someone else should take a look after you've made revisions so we can get it in this sprint if possible (the sprint ends on Friday). Thanks again for working on this!

@njx
Adobe Systems Incorporated member
njx commented Feb 28, 2013

Ah, I wonder if that delay could be due to the issue that @TuckerWhitehouse mentioned. If the JSONP stuff is causing a <script> tag to be inserted, that could be happening synchronously within the $.getJSON() call--perhaps that causes some sort of delay. If github.com does indeed have a cross-domain policy that makes this unnecessary, then it should be fine to switch to a direct fetch (by removing the ?callback=?).

@TomMalbran

@njx Thanks for the review. I just pushed the fixes for all your comments, and yes @TuckerWhitehouse was right, it does feel faster without the JSONP stuff.

The only thing that I think is missing is maybe an error response when something went wrong. Just removing the spinner and filling the contributors space with an error message.

@njx njx commented on the diff Mar 1, 2013
src/help/HelpCommandHandlers.js
var buildInfo;
-
+
+
+ /**
+ * @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.
+ * @return {$.Promise} jQuery Promise object that is resolved or rejected after the information is fetched.
+ */
+ function _getContributorsInformation() {
+ var result = new $.Deferred();
+
+ $.getJSON(brackets.config.contributors_url)
@njx
Adobe Systems Incorporated member
njx added a note Mar 1, 2013

This could be simplified now to just return $.getJSON(brackets.config.contributors_url), I think (in fact, you probably don't really need this function anymore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx njx commented on the diff Mar 1, 2013
src/help/HelpCommandHandlers.js
+ if (this.complete) {
+ $(this).trigger("load");
+ }
+ });
+
+ // Create a link for each contributor image to their github account
+ $contributors.on("click", "img", function (e) {
+ 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)) {
+ NativeApp.openURLInDefaultBrowser(url);
+ }
+ }
+ });
+ });
@njx
Adobe Systems Incorporated member
njx added a note Mar 1, 2013

Yup, there should be a fail() case here that turns the spinner off and maybe adds something like: "Lots of people (but we can't access github right now to find out who)."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@njx
Adobe Systems Incorporated member
njx commented Mar 1, 2013

Re-reviewed. I agree we should have an error response, but I think we could go ahead and merge this without that--I can file a bug to get that in next sprint as well as clean up the other code review notice. But if you happen to be online now :), feel free to go ahead and push fixes. I'll wait a little bit before merging to see if you push any fixes.

(Also, I'm not sure what's up with the Travis failure--it looks spurious to me. If you do another push, we'll see if it fails again.)

@njx
Adobe Systems Incorporated member
njx commented Mar 1, 2013

I'm going to go ahead and merge--will file a bug for the remaining issues. Thanks again for working on this.

@njx njx merged commit d4d81e3 into adobe:master Mar 1, 2013

1 check failed

Details default The Travis build could not complete due to an error
@njx
Adobe Systems Incorporated member
njx commented Mar 1, 2013

Followup bug filed as #3012

@TomMalbran TomMalbran deleted the TomMalbran:tom/contributors-list branch Mar 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.