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

NEW - Ahmed Essam - GSOD #898

Closed
wants to merge 4 commits into from
Closed

NEW - Ahmed Essam - GSOD #898

wants to merge 4 commits into from

Conversation

Hanaffi
Copy link

@Hanaffi Hanaffi commented May 26, 2021

I made new PR for all files better than last approach

@Hanaffi Hanaffi requested a review from a team as a code owner May 26, 2021 11:17
@Hanaffi
Copy link
Author

Hanaffi commented May 26, 2021

@huan @Rohitesh-Kumar-Jain

@Rohitesh-Kumar-Jain
Copy link
Contributor

@huan @Rohitesh-Kumar-Jain

We still have this error :

# all asset files should be put into folder `/assets/YYYY/MM-slug-...-slug/` (slugs should be the same as the post)
not ok 13 "/assets/2021/05-gsod-2021-volunteering-proposal/gsod_s.webp" from "jekyll/_posts/2021-05-05-ahmed-essam-gsod-2021-volunteering-proposal.md" should be save to "assets/2021/05-ahmed-essam-gsod-2021-volunteering-proposal/"
  ---
    operator: fail
    at: Test.<anonymous> (/home/runner/work/wechaty.js.org/wechaty.js.org/tests/jekyll/asset-file-location.spec.ts:58:11)
    stack: |-
      Error: "/assets/2021/05-gsod-2021-volunteering-proposal/gsod_s.webp" from "jekyll/_posts/2021-05-05-ahmed-essam-gsod-2021-volunteering-proposal.md" should be save to "assets/2021/05-ahmed-essam-gsod-2021-volunteering-proposal/"
          at Test.assert [as _assert] (/home/runner/work/wechaty.js.org/wechaty.js.org/node_modules/tape/lib/test.js:228:54)
          at Test.bound [as _assert] (/home/runner/work/wechaty.js.org/wechaty.js.org/node_modules/tape/lib/test.js:80:32)
          at Test.fail (/home/runner/work/wechaty.js.org/wechaty.js.org/node_modules/tape/lib/test.js:322:10)
          at Test.bound [as fail] (/home/runner/work/wechaty.js.org/wechaty.js.org/node_modules/tape/lib/test.js:80:32)
          at Test.<anonymous> (/home/runner/work/wechaty.js.org/wechaty.js.org/tests/jekyll/asset-file-location.spec.ts:58:11)
  ...

Just fix this and we are good to go

@Hanaffi
Copy link
Author

Hanaffi commented May 26, 2021

@Rohitesh-Kumar-Jain Last check :( ?

@Rohitesh-Kumar-Jain
Copy link
Contributor

@Rohitesh-Kumar-Jain Last check :( ?

don't worry, I can check more : )

Copy link
Contributor

@Rohitesh-Kumar-Jain Rohitesh-Kumar-Jain left a comment

Choose a reason for hiding this comment

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

Congratulations CI turned green 🎉 🥳

@Rohitesh-Kumar-Jain
Copy link
Contributor

ping @huan

@Hanaffi
Copy link
Author

Hanaffi commented May 28, 2021

@huan Hey Huan, Can you merge? Its finally green :)

@Hanaffi
Copy link
Author

Hanaffi commented Jun 8, 2021

@huan Can you merge please?

- volunteering
- gsod
- proposal
image: /assets/2021/05-ahmed-essam-gsod-2021-volunteering-proposal/gsod_s.webp
Copy link
Member

Choose a reason for hiding this comment

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

Please follow our file name rule:

Create a markdown file in jekyll/_posts folder. The file name should follow the format YYYY-MM-DD-your-blog-slug.md. For example 2016-12-03-welcome-to-wechaty.md

Please change gsod_s.webp to gsod-s.webp

@lijiarui
Copy link
Member

hi @Rohitesh-Kumar-Jain As GSoD Project PR Review Rules in https://github.com/wechaty/wechaty.js.org/issues/1045

This PR should follow our rules and get enough approval, then I will review this PR. So I remove the ready to merge tag and feel free to add this tag when it gets enough approval.

@lijiarui
Copy link
Member

Hi @Hanaffi , Since the .webp format has lots of advantages and the safari has supported it last year which means all the modern browsers will be able to compatible with it.

@huan has just added a new unit test for enforcing the image format: the CI will not be passed if we are using other image formats than the perfect .webp.

Please make CI green to adjust the new rule, see https://github.com/wechaty/wechaty.js.org/issues/1035

@Rohitesh-Kumar-Jain
Copy link
Contributor

Hi @Hanaffi , would you like to push more commits to turning the CI green?

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.

Please check the CI and make sure it turns green before we can continue reviewing this PR, thanks!

See: https://github.com/wechaty/wechaty.js.org/pull/898/checks?check_run_id=3035429136#step:8:65

@lijiarui
Copy link
Member

ping @Hanaffi

@lijiarui
Copy link
Member

ping @Hanaffi

@Rohitesh-Kumar-Jain
Copy link
Contributor

@Hanaffi feel free to reopen the PR, and push more commits to it :)

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.

4 participants