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

[GSoD 2021] Converted all Jekyll blog contributors images to webp #1072

Merged
merged 10 commits into from
Jul 24, 2021
Merged

[GSoD 2021] Converted all Jekyll blog contributors images to webp #1072

merged 10 commits into from
Jul 24, 2021

Conversation

iamrajiv
Copy link
Member

@iamrajiv iamrajiv commented Jul 13, 2021

Fixes: #1035
Related: #1012

Screenshot 2021-07-13 at 8 33 06 AM

iamrajiv added 2 commits July 13, 2021 17:53
Signed-off-by: Rajiv Ranjan Singh <rajivperfect007@gmail.com>
Signed-off-by: Rajiv Ranjan Singh <rajivperfect007@gmail.com>
@iamrajiv iamrajiv requested a review from a team as a code owner July 13, 2021 12:34
Signed-off-by: Rajiv Ranjan Singh <rajivperfect007@gmail.com>
@nibble0101
Copy link
Contributor

nibble0101 commented Jul 13, 2021

Hi, @huan can you take a look at this PR. Also, all things are correct not don know why CI is saying Error: should exist on GitHub. Is there any limit on the number of changes in a single PR?

It seems that error is due to the fact that actual requests are being made to GitHub when running these tests. You can look at these lines of code. Given the number of files you have changed (I suspect you changed the image formats for all contributors to webp), the number of requests made to GitHub while running the tests is as many as the number of contributors. That is why after making 40 - 60 requests to GitHub, you will start getting statusText of too many requests in the response making the tests to start failing. Ideally a person adding him/herself as contributor will necessitate making one fetch request when testing the existence of their account. The kind of changes in your PR was perhaps never envisaged.

You can add the code below here and then run the tests on your computer.

console.log('statusText', response.statusText)
console.log('ok', response.ok)

When I did the same, this is what I got. Though you might need to comment out these lines of code to avoid having a congested terminal.
image

I believe one of the solutions could be to split the PR so that each PR is changing image format for not more than 50 contributors, though I have not really tried whether that will work or not. Another option is to merge ignoring the failing tests.
yes splitting the PR might help out @iamrajiv

@shraddhavp shraddhavp requested a review from huan July 14, 2021 04:51
Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you so much for converting all of those image files to .webp, it's really a great improvement and for our website!

@huan
Copy link
Member

huan commented Jul 19, 2021

@iamrajiv Yes, I agree with you that the best approach is ignoring the failing tests, and thanks to @nibble0101 for pointing the reason for the failing CI.

I noticed that there are two files are conflict with the main branch, please fix those conflicts then I believe we will be able to merge this epic PR!

iamrajiv added 5 commits July 19, 2021 23:23
Signed-off-by: Rajiv Ranjan Singh <rajivperfect007@gmail.com>
Signed-off-by: Rajiv Ranjan Singh <rajivperfect007@gmail.com>
@huan huan merged commit e03267e into wechaty:master Jul 24, 2021
@huan
Copy link
Member

huan commented Jul 24, 2021

@iamrajiv thanks for the notification.

Let's merge this PR first, and please have a double check after this PR has been published.

Appreciate to all your efforts to webpify our images!

@iamrajiv iamrajiv changed the title Converted all Jekyll blog contributors images to webp [GSoD 2021] Converted all Jekyll blog contributors images to webp Jul 29, 2021
@iamrajiv iamrajiv changed the title [GSoD 2021] Converted all Jekyll blog contributors images to webp [GSoD 2021] Converted all Jekyll blog contributors images to webp Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce all images in wechaty website to use .webp format
4 participants