search

AccessibleForAll / AccessibleWebDev

A resource for developers wanting to learn the basics of accessibility
https://accessibleweb.dev
MIT License
172 stars 95 forks source link

Reduced Maintainer's image load time #355

Closed SaptarshiSarkar12 closed 1 year ago

SaptarshiSarkar12 commented 1 year ago

Describe your changes

Replaced @EmmaDawsonDev's GitHub profile picture link with https://avatars.githubusercontent.com/u/57045550?v=4&s=150 and @ctoffanin's GitHub profile picture link with https://avatars.githubusercontent.com/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)

Final Output

Link to issue

Closes #354

Checklist before requesting a review

github-actions[bot] commented 1 year ago

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!

SaptarshiSarkar12 commented 1 year ago

@EmmaDawsonDev @ctoffanin Please review this PR.

SaptarshiSarkar12 commented 1 year ago

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.

EmmaDawsonDev commented 1 year ago

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!

SaptarshiSarkar12 commented 1 year ago

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!

SaptarshiSarkar12 commented 1 year ago

@EmmaDawsonDev I have reverted the commits regarding yarn.lock and .yarnrc.yml. Please re-review this PR.

at-the-vr commented 1 year ago

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?

Yeah its interesting that specifying https://github.com/emmadawsondev.png | width=150 in maintainers.ts results in a 406 Error because its a redirect to avatar.githubuser route, but with avatar.githubuser. . . . route you have the s parameter available for resizing.

SaptarshiSarkar12 commented 1 year ago

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?

Yeah its interesting that specifying https://github.com/emmadawsondev.png | width=150 in maintainers.ts results in a 406 Error because its a redirect to avatar.githubuser route, but with avatar.githubuser. . . . route you have the s parameter available for resizing.

Yes, you are right @at-the-vr !

SaptarshiSarkar12 commented 1 year ago

completely valid, solid workaround :+1:

Thank you for your review @at-the-vr :grin: !

SaptarshiSarkar12 commented 1 year ago

@EmmaDawsonDev Please review this PR.

SaptarshiSarkar12 commented 1 year ago

@EmmaDawsonDev Please review this PR.

@EmmaDawsonDev

SaptarshiSarkar12 commented 1 year ago

Looks great, thanks for your help!

Thank you for the merge @EmmaDawsonDev :grinning: !