fix: image sourced from github #680
Conversation
✅ Deploy Preview for ja-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for new-eslint ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for es-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for zh-hans-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for hi-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for fr-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for de-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pt-br-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
tools/add-author.js
Outdated
| @@ -46,7 +46,7 @@ async function fetchUserProfile() { | |||
| name: profile.name, | |||
| title: "Guest Author", | |||
| website: profile.blog, | |||
| avatar_url: profile.avatar_url, | |||
| avatar_url: `${profile.avatar_url.replace("private-", "").split("?")[0]}?v=4`, | |||
There was a problem hiding this comment.
| avatar_url: `${profile.avatar_url.replace("private-", "").split("?")[0]}?v=4`, | |
| avatar_url: `${profile.avatar_url.replace("private-", "").split("?")[0]}?v=4`, |
We could consider adding a fallback image in case the actual image isn't found, though I'm not sure if GitHub itself provides a fallback image.
There was a problem hiding this comment.
sounds good to me, should we use the fallback image which we are using for sponsors? this one
There was a problem hiding this comment.
I am 👍 let's get other's opinions too.
There was a problem hiding this comment.
This fallback also looks good to me.
There was a problem hiding this comment.
Added fallback image.
tools/fetch-team-data.js
Outdated
| @@ -96,7 +96,7 @@ async function fetchUserProfile(username) { | |||
| : profile.blog | |||
| ? `https://${profile.blog}` | |||
| : profile.blog, | |||
| avatar_url: profile.avatar_url, | |||
| avatar_url: `${profile.avatar_url.replace("private-", "").split("?")[0]}?v=4`, | |||
There was a problem hiding this comment.
Can we extract this into a shared place (e.g., tools/utils.js) as a function so that we don't have to repeat the same logic in multiple files?
There was a problem hiding this comment.
Done! tools/utils.js has been created.
tools/fetch-team-data.js
Outdated
| @@ -96,7 +96,7 @@ async function fetchUserProfile(username) { | |||
| : profile.blog | |||
| ? `https://${profile.blog}` | |||
| : profile.blog, | |||
| avatar_url: profile.avatar_url, | |||
| avatar_url: `${profile.avatar_url.replace("private-", "").split("?")[0]}?v=4`, | |||
There was a problem hiding this comment.
Some URLs have u parameter:
eslint.org/src/_data/sponsors.json
Lines 76 to 77 in 617c196
Any ideas about what it represents?
Should we maybe remove just the jwt parameter and leave the others?
There was a problem hiding this comment.
Some URLs have u parameter:
Can it stands for unique? even though without this parameter the results are same.
Also seems links are already fixed by commit e385d5f should we still apply this fix?
There was a problem hiding this comment.
Also seems links are already fixed by commit e385d5f should we still apply this fix?
I also noticed that it might be a good idea to wait a couple of days. If GitHub is still testing features with a subset of users, it could potentially break again. As of now, we might not be part of the experiment, but if they do a full rollout, we could be impacted.
There was a problem hiding this comment.
I think it would be good to continue working on this PR and have it ready (or perhaps even merge it) in case the problem happens again. Unless they've decided not to continue with the change, but I couldn't find any mention of that in https://github.com/orgs/community/discussions/147297.
There was a problem hiding this comment.
Should we maybe remove just the jwt parameter and leave the others?
should we work on this approach? as we can see both links with and without u token gives the same result:
https://avatars.githubusercontent.com/u/53535748?u=d1e55d7661d724bf2281c1bfd33cb8f99fe2465f&v=4
There was a problem hiding this comment.
I think it's better to remove the JWT parameter and retain the others.
Prerequisites checklist
What is the purpose of this pull request?
Constructing old avatar url from the new one.
What changes did you make? (Give an overview)
Did omit the
private-word andjwttoken from the URL of image fetched from github.Related Issues
Fixes #679
Is there anything you'd like reviewers to focus on?