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. |
amareshsm
left a comment
amareshsm
left a comment
There was a problem hiding this comment.
LGTM. Thanks. Leaving it open for 2nd review.
tools/add-author.js
Outdated
| 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
| ? `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
| ? `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.
|
I think this is a bit more complicated than we need. We can easily calculate the correct URL in our fetch scripts by using the user's ID. I put this together: |
So, i think i should revert the avatar link related changes and keep only fallback related changes here? |
|
I'm not sure we need the fallback changes. |
| <img class="blog-post__author-photo" src="{% if params.authorImage %}{{ params.authorImage }} {% else %}../../../assets/images/people/_placeholder-avatar.svg{% endif %}" alt="{{ params.authorName }}" width="40" height="40" loading="lazy"> | ||
| {% set fallback_image = '/assets/images/people/_placeholder-avatar.svg' %} | ||
| <img class="blog-post__author-photo" src="{{ params.image if params.image else fallback_image }}" alt="{{ params.authorName }}" width="40" height="40" loading="lazy" onerror="this.src = '{{fallback_image}}'"> |
There was a problem hiding this comment.
I'm not sure we need the fallback changes.
We can see here a fallback image is already applied but actually it's not working, can it be considered as a bug?
There was a problem hiding this comment.
Yes, I think that can be considered a bug. I just don't think we need to add fallback images to places where we didn't have them before.
There was a problem hiding this comment.
I guess the fallback image was working as intended.
There was a problem hiding this comment.
Fallback images was only working for sponsors in All backers section as you can see in images provided in issue template of #679
There was a problem hiding this comment.
I just don't think we need to add fallback images to places where we didn't have them before.
reverted the changes.
Tanujkanti4441
changed the title
5 participants
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?