-
Notifications
You must be signed in to change notification settings - Fork 95
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.
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
Reduced Maintainer's image load time #355
Reduced Maintainer's image load time #355
Conversation
Replaced @EmmaDawsonDev's GitHub profile picture link with **https://github.com/avatars/u/57045550?v=4&s=150** and @ctoffanin's GitHub profile picture link with **https://github.com/avatars/u/1682188?v=4&s=150**. This github profile picture cdn link will provide a **150 x 150** image which does not make any visual difference for the end user, but, it reduces the network payload by about 87%.
Hello @SaptarshiSarkar12, thanks for raising a pull request in this project. The maintainers of this project are volunteers so please be understanding if it takes time before you get a response. We still appreciate your help with creating pull requests! |
@EmmaDawsonDev @ctoffanin Please review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the commit and then only recommit changes to the relevant file and not the yarn files.
Also, is there a reason why the whole image url needs changing?
@EmmaDawsonDev Please check my comments in the reviews made by @at-the-vr . If you want to remove those files from my commit, then I shall proceed. Yeah @EmmaDawsonDev , the whole url needs to be changed as the size parameter does not work with the previous github profile picture url. |
You did not add any packages so you do not need to commit the yarn files. Please only commit the changes to the maintainers data, thanks! |
Ok, deleting the yarn lock and yarnrc yml files. Thank you @EmmaDawsonDev! |
@EmmaDawsonDev I have reverted the commits regarding yarn.lock and .yarnrc.yml. Please re-review this PR. |
Yeah its interesting that specifying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely valid, solid workaround 👍
Yes, you are right @at-the-vr ! |
Thank you for your review @at-the-vr 😁 ! |
@EmmaDawsonDev Please review this PR. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for your help!
Thank you for the merge @EmmaDawsonDev 😀 ! |
Describe your changes
Replaced @EmmaDawsonDev's GitHub profile picture link with https://github.com/avatars/u/57045550?v=4&s=150 and @ctoffanin's GitHub profile picture link with https://github.com/avatars/u/1682188?v=4&s=150. This github profile picture cdn link will provide a 150 x 150 image which does not make any visual difference for the end user, but, it reduces the network payload by about 87%.
Screenshots - If Any (Optional)
Link to issue
Closes #354
Checklist before requesting a review
and it shows no errors.
Additional Information (Optional)
The resultant file size changes :