Adjust profile image dimensions for Fb and Twitter #382
Conversation
|
@igorauad 0.4.0 is preferred so they get merged into the new version. I'm okay with these changes, but even though it's so little I'd prefer the username and image changes to be in 2 separate PRs just because they are pretty different things. Also, is there any way to test these OAuth flows and configs? I couldn't think of a good way, but we should really be testing these just like we test our own API because changes may happen without us knowing and the tests would let us know. |
igorauad
force-pushed
the
providerImages
branch
from
b2f7529 to
92e80a2
Compare
igorauad
changed the title
|
You are right, @ilanbiala ! I will separate them. This one has only the image changes now. Testing OAuth would be interesting, I don't know an easy way of testing either. I have tested for Fb here I realized it was not working due to Maybe at least a standard |
|
LGTM. @lirantal @NeverOddOrEven looks good? |
| @@ -30,7 +30,7 @@ module.exports = function(config) { | |||
| displayName: profile.displayName, | |||
| email: profile.emails[0].value, | |||
| username: profile.username, | |||
| profileImageURL: (profile.photos && profile.photos.length) ? profile.photos[0].value : undefined, | |||
| profileImageURL: (profile.id) ? 'https:graph.facebook.com/' + profile.id + '/picture?type=large' : undefined, | |||
There was a problem hiding this comment.
is it actually https:graph or is that a typo and should read really https://graph...?
There was a problem hiding this comment.
It seems to be a typo, but I am actually using it this way and it is working. I will change it as soon as we decide whether //graph should be used.
|
@igorauad is the change there means that we won't be storing the profile image on the meanjs project but rather take it from facebook using the URL there? If so, then websites which are enabling http:// and not SSL might have browsers block SSL communication (and vice-versa) due to security issues |
|
Please use |
|
@lirantal we were not storing the profile image, but rather storing another URL from @lirantal @simison , so, shall I change to //graph? This would work with |
|
@igorauad I'm unable to test, but please do change it to //graph and test it to say if it works or not before committing. |
igorauad
force-pushed
the
providerImages
branch
from
92e80a2 to
2025b0d
Compare
|
@lirantal I've tested. The protocol-relative URL works. |
igorauad
force-pushed
the
providerImages
branch
2 times, most recently
from
c92d3fa to
0a1c33a
Compare
| @@ -26,7 +26,7 @@ module.exports = function(config) { | |||
| var providerUserProfile = { | |||
| displayName: profile.displayName, | |||
| username: profile.username, | |||
| profileImageURL: (profile.photos && profile.photos.length) ? profile.photos[0].value : undefined, | |||
| profileImageURL: (profile.photos && profile.photos.length) ? profile.photos[0].value.replace('normal', 'bigger') : undefined, | |||
There was a problem hiding this comment.
Is there a case where a user won't have a photo that you have to check that it exists first? I would think even the bird would be the first profile picture, no? And is [0] the latest or the first?
There was a problem hiding this comment.
I think FB and Twitter would return a default image if the user hasn't uploaded one. I haven't tested this though.
https://developers.facebook.com/docs/graph-api/reference/v2.2/user/picture
Looks like is_silhouette can be used to detect the if the default picture been replaced or not.
On Feb 1, 2015, at 11:18 AM, Ilan Biala notifications@github.com wrote:
In modules/users/server/config/strategies/twitter.js:
@@ -26,7 +26,7 @@ module.exports = function(config) {
var providerUserProfile = {
displayName: profile.displayName,
username: profile.username,
profileImageURL: (profile.photos && profile.photos.length) ? profile.photos[0].value : undefined,profileImageURL: (profile.photos && profile.photos.length) ? profile.photos[0].value.replace('normal', 'bigger') : undefined,—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
Right, but is that in the array? If it is, then we don't need to check the length and existence.
There was a problem hiding this comment.
Yes, it is in the array, @ilanbiala . I've just tested here. Twitter returns
[ { value: 'https://abs.twimg.com/sticky/default_profile_images/default_profile_0_normal.png' } ]
For Fb, use the Graph API. For twitter, use the 'bigger' profile image. Larger profile images (like the one provided by Google) could provide more flexibility.
igorauad
force-pushed
the
providerImages
branch
from
0a1c33a to
12766c1
Compare
|
LGTM, @lirantal any suggestions? |
|
@ilanbiala I'm ok with it. |
Use larger profile images for Facebook and Twitter
@ilanbiala please, let me know where I should be suggesting these PRs. Shall I do it for 0.4.0, master or both?
Regarding image sizes, I just faced a need for larger dimensions, so I changed the profileImageUrl for FB and Twitter. Thought perhaps this should be the default. What do you think?