Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLOSED] Contributors List on the About Dialog #2771

Open
core-ai-bot opened this issue Aug 29, 2021 · 16 comments
Open

[CLOSED] Contributors List on the About Dialog #2771

core-ai-bot opened this issue Aug 29, 2021 · 16 comments

Comments

@core-ai-bot
Copy link
Member

Issue by TomMalbran
Friday Feb 22, 2013 at 11:52 GMT
Originally opened as adobe/brackets#2934

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.

TomMalbran included the following code: https://github.com/adobe/brackets/pull/2934/commits
@core-ai-bot
Copy link
Member Author

Comment by WebsiteDeveloper
Sunday Feb 24, 2013 at 10:44 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Feb 24, 2013 at 21:07 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by adrocknaphobia
Monday Feb 25, 2013 at 23:10 GMT

@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).

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Feb 26, 2013 at 01:09 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by adrocknaphobia
Tuesday Feb 26, 2013 at 01:57 GMT

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

Thanks!

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Tuesday Feb 26, 2013 at 03:10 GMT

Replaced $.each with forEach as Peter mentioned.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Feb 27, 2013 at 19:47 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Feb 27, 2013 at 19:50 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Feb 27, 2013 at 19:51 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Wednesday Feb 27, 2013 at 22:15 GMT

  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!

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 28, 2013 at 03:35 GMT

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!

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Feb 28, 2013 at 03:38 GMT

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=?).

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Feb 28, 2013 at 04:01 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Mar 01, 2013 at 21:24 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Mar 01, 2013 at 21:53 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Mar 01, 2013 at 21:56 GMT

Followup bug filed as #3012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot