Skip to content
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

Conversation

SaptarshiSarkar12
Copy link
Contributor

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)

Final Output

Link to issue

Closes #354

Checklist before requesting a review

  • I have performed a self-review of my code.
  • Followed the repository's Contributing Guidelines.
  • I ran the app and tested it locally to verify that it works as expected.
  • I have checked my code with an automatic accessibility tool such as Axe Dev Tools or Wave
    and it shows no errors.

Additional Information (Optional)

The resultant file size changes :

User Before After
@EmmaDawsonDev 226.7 kB 31.6 kB
@ctoffanin 21.3 kB 4 kB

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%.
@github-actions
Copy link

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
Copy link
Contributor Author

@EmmaDawsonDev @ctoffanin Please review this PR.

.yarnrc.yml Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
Copy link
Member

@EmmaDawsonDev EmmaDawsonDev left a 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?

data/maintainers.ts Outdated Show resolved Hide resolved
@SaptarshiSarkar12
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@at-the-vr
Copy link
Contributor

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.

Copy link
Contributor

@at-the-vr at-the-vr left a 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 👍

@SaptarshiSarkar12
Copy link
Contributor Author

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
Copy link
Contributor Author

completely valid, solid workaround 👍

Thank you for your review @at-the-vr 😁 !

@SaptarshiSarkar12
Copy link
Contributor Author

@EmmaDawsonDev Please review this PR.

@SaptarshiSarkar12
Copy link
Contributor Author

@EmmaDawsonDev Please review this PR.

@EmmaDawsonDev

Copy link
Member

@EmmaDawsonDev EmmaDawsonDev left a 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!

@EmmaDawsonDev EmmaDawsonDev merged commit c81e001 into AccessibleForAll:main Jul 23, 2023
@SaptarshiSarkar12
Copy link
Contributor Author

Looks great, thanks for your help!

Thank you for the merge @EmmaDawsonDev 😀 !

@SaptarshiSarkar12 SaptarshiSarkar12 deleted the 354-reduce-maintainer-image-size branch July 23, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintainer image load size can be reduced
3 participants