peidevs / Scooter

Angular Port of Scooter
Apache License 2.0
0 stars 1 forks source link

Add profile picture fetch and display for players #28

Closed Qcode closed 8 years ago

Qcode commented 8 years ago

This allows profile pictures to be displayed along with names on Scooter. This should allow people in the audience to find themselves faster. The layout looks great on the sticky note theme, but a bit worse on the original Scooter theme.

swhalley commented 8 years ago

Hey, I did have a question about a behavior. What happens if you use the "Add Player" option in the configuration menu? I noticed the default image is provided in the meetup loader. But if a new Player object is constructed, via the "Add Player" function there is no default image. What will happen when the tag tries to render with no value?

swhalley commented 8 years ago

Also, as a possible enhancement to this, I would like to see a configuration added which allows the picture to be shown/hidden

Qcode commented 8 years ago

I checked, previously it would render no picture if you added with the "Add Player" GUI. I've added another line in the Player model which should fix that, makes it use the default image. The configuration, I could do that. Would you want that on an individual player basis or just a global show/hide pictures scale?

swhalley commented 8 years ago

the configuration I would do on a game basis, not player. 1 checkbox would be enough.

swhalley commented 8 years ago

The logic around photo_link is duplicated in the Configuration Modal and meetup Service classes (and now Player in your latest revision). For a player pulled from meetup, they will already have the attentdee.member_photo.photo_link link, so we really only need to default for Guests (the +1s) and Added players. I don't think we need the logic to exist in the configuration and meetup service modules. Something like this could be pushed off to the view. Like what you did in the player object, you could do something similar in the view.

<img ng-src="{{attendee.photo_link || 'http://img2.meetupstatic.com/img/458386242735519287330/noPhoto_50.png'}}"/>

so for the +1s and added players, default their photo_link to null and then let the view do the display :) Here is a SO article showing how the ng-src tag can be used with conditionals http://stackoverflow.com/questions/13999659/conditionally-change-img-src-based-on-model-data

side note, consider using the property thumb_link instead. This image size may be smaller resulting in quicker load times. I didn't test this though

Qcode commented 8 years ago

I've taken some time and attempted to clean this up a bit more. Default profile picture logic has been pushed off to the view now. I still unfortunately have to check if member_photo exists before referencing the thumb link, but at least there's no longer the default image link thrown around everywhere. I've also renamed all instances of photo_link to thumb_link to avoid confusion. I was using thumb_link originally but called the variable photo_link, in hindsight I can see why that was a stupid idea.

I've also added in that display toggle you wanted, let me know if that code needs any revision.

swhalley commented 8 years ago

config.html Question - Can you use ng-model instead of ng-click for the toggleProfilePictures variable? Bind it, instead of calling the function? I am just asking a question here, because I haven't looked at how angular works with check boxes. Honestly I don't know which is the "angular way".

MeetupService.retrieveGuests() For the +1's don't worry about trying to determine a picture. From a behavior standpoint, if you bring a guest (+1) we shouldn't see your face for them. So in the retrieveGuests() function I think you can remove those changes and it should work. This means the +1's would get the generic face, which I think is better behavior for the app?

Otherwise looks great :)

Qcode commented 8 years ago

Yeah, that's possible. Originally I thought that didn't work because I was declaring the variable inside ConfigurationModal.js, and didn't realize that it got reloaded each time the config page was opened. When I figured that out I forgot to consider ng-model again.

Ah, I didn't realize that function was for the +1s until just now. I probably should have figured that out before I went adding code into it... Nevertheless, I've removed that logic from there now. Thanks for helping me out with this, I've had a lot of fun contributing!

swhalley commented 8 years ago

One last change and then I will approve. You left an extra comma in the meetupService class.

Qcode commented 8 years ago

Extra comma has been removed.

swhalley commented 8 years ago

accepted and merged changes to master.

As a side note, issue #17 I am going to add support to save a user preference between games. Look for that in the future

swhalley commented 8 years ago

Great Work :+1: