[Feature] Render error card in same theme as requested card #3259

[notpritam]

Hello , i have add the feature

here is the before and after

return res.send(renderError(err.message, err.secondaryMessage));

i have added it by introducing a new params in renderError function :-

    return res.send(
      renderError(err.message, err.secondaryMessage, {
        title_color,
        text_color,
        bg_color,
        border_color,
        theme,
      }),
    );

Previous
Example 1
Previous 2
Example 2

[vercel]

@notpritam is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (33beba5) 98.40% compared to head (b51dd60) 98.42%.
Report is 16 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3298      +/-   ##
==========================================
+ Coverage   98.40%   98.42%   +0.02%     
==========================================
  Files          26       26              
  Lines        6002     6114     +112     
  Branches      534      534              
==========================================
+ Hits         5906     6018     +112     
  Misses         93       93              
  Partials        3        3              

Files	Coverage Δ
api/gist.js	98.07% <100.00%> (+0.34%)	⬆️
api/index.js	100.00% <100.00%> (ø)
api/pin.js	98.30% <100.00%> (+0.43%)	⬆️
api/top-langs.js	100.00% <100.00%> (ø)
src/common/utils.js	99.02% <100.00%> (+0.03%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[qwerty541]

Hey, @notpritam! Thanks for your pull request and for your patience! Generally your code looks well, but it would be nice if this feature can be covered with test. Also i'm okay with adding test in future pull requests since i see that this feature works on test deployment that you provided. I give my approval to this pull request and mark it with hacktoberfest-accepted label. We will merge it when any other maintainer also gives his approval.

[notpritam]

Hey, @notpritam! Thanks for your pull request and for your patience! Generally your code looks well, but it would be nice if this feature can be covered with test.

Thanks for your response , i dont have any experience creating test so i didnt created that.
if you want to me create the test for this also, i will love to do that but i will need a little longer time, cause this will be my first time creating any tests.

[qwerty541]

Hey, @notpritam! Thanks for your pull request and for your patience! Generally your code looks well, but it would be nice if this feature can be covered with test.

Thanks for your response , i dont have any experience creating test so i didnt created that. if you want to me create the test for this also, i will love to do that but i will need a little longer time, cause this will be my first time creating any tests.

As I said, this is desirable, not required, since you provided a link on test vercel deployment. I hope that adding this test case into https://github.com/anuraghazra/github-readme-stats/blob/master/tests/api.test.js file will be enough, may be some changes will be needed, or i can add it personally in future PRs.

it("should render error card in provided theme", async () => {
    const { req, res } = faker({ theme: "merko" }, error);

    await api(req, res);

    expect(res.setHeader).toBeCalledWith("Content-Type", "image/svg+xml");
    expect(res.send).toBeCalledWith(
      renderError(
        error.errors[0].message,
        "Make sure the provided username is not an organization",
        { theme: "merko" },
      ),
    );
  });

I also noticed that there is some conflicts with master branch, can you please resolve it?

[notpritam]

Hey, @notpritam! Thanks for your pull request and for your patience! Generally your code looks well, but it would be nice if this feature can be covered with test.

Thanks for your response , i dont have any experience creating test so i didnt created that. if you want to me create the test for this also, i will love to do that but i will need a little longer time, cause this will be my first time creating any tests.

As I said, this is desirable, not required, since you provided a link on test vercel deployment. I hope that adding this test case into https://github.com/anuraghazra/github-readme-stats/blob/master/tests/api.test.js file will be enough, may be some changes will be needed, or i can add it personally in future PRs.

it("should render error card in provided theme", async () => {
    const { req, res } = faker({ theme: "merko" }, error);

    await api(req, res);

    expect(res.setHeader).toBeCalledWith("Content-Type", "image/svg+xml");
    expect(res.send).toBeCalledWith(
      renderError(
        error.errors[0].message,
        "Make sure the provided username is not an organization",
        { theme: "merko" },
      ),
    );
  });

I also noticed that there is some conflicts with master branch, can you please resolve it?

Hii, @qwerty541 , i have fixed the conflicts, and also updated the code, please review the and merge it ,

thanks...

[notpritam]

@rickstaa and @anuraghazra , can you please look over the code and approve it ,
thanks a lot...

[qwerty541]

Hey, @notpritam! Thanks for your pull request and for your patience! Generally your code looks well, but it would be nice if this feature can be covered with test.

Thanks for your response , i dont have any experience creating test so i didnt created that. if you want to me create the test for this also, i will love to do that but i will need a little longer time, cause this will be my first time creating any tests.

As I said, this is desirable, not required, since you provided a link on test vercel deployment. I hope that adding this test case into https://github.com/anuraghazra/github-readme-stats/blob/master/tests/api.test.js file will be enough, may be some changes will be needed, or i can add it personally in future PRs.

it("should render error card in provided theme", async () => {
    const { req, res } = faker({ theme: "merko" }, error);

    await api(req, res);

    expect(res.setHeader).toBeCalledWith("Content-Type", "image/svg+xml");
    expect(res.send).toBeCalledWith(
      renderError(
        error.errors[0].message,
        "Make sure the provided username is not an organization",
        { theme: "merko" },
      ),
    );
  });

I also noticed that there is some conflicts with master branch, can you please resolve it?

Hii, @qwerty541 , i have fixed the conflicts, and also updated the code, please review the and merge it ,

thanks...

@notpritam I have found two more problems after resolving conflicts, please fix it.

[qwerty541]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

[rickstaa]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

Tested and approved! @notpritam, thanks a lot for this outstanding contribution ❤️‍🔥!

@qwerty541 we can merge this when tests are added 👍🏻.

[notpritam]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

Tested and approved! @notpritam, thanks a lot for this outstanding contribution ❤️‍🔥!

@qwerty541 we can merge this when tests are added 👍🏻.

thanks a lot, means a lot for me coming from you..

[qwerty541]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

Tested and approved! @notpritam, thanks a lot for this outstanding contribution ❤️‍🔥!

@qwerty541 we can merge this when tests are added 👍🏻.

@rickstaa Since @notpritam tells that he has no experience in tests addition, may be we can merge this and i will add tests in next pull request. Anyway this feature was tested on Vercel test deployment.

[rickstaa]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

Tested and approved! @notpritam, thanks a lot for this outstanding contribution ❤️‍🔥!
@qwerty541 we can merge this when tests are added 👍🏻.

@rickstaa Since @notpritam tells that he has no experience in tests addition, may be we can merge this and i will add tests in next pull request. Anyway this feature was tested on Vercel test deployment.

Sounds okay to me 👍🏻. Thanks!

[qwerty541]

@rickstaa and @anuraghazra , can you please look over the code and approve it , thanks a lot...

I already requested their review #3298 (comment)

Tested and approved! @notpritam, thanks a lot for this outstanding contribution ❤️‍🔥!
@qwerty541 we can merge this when tests are added 👍🏻.

@rickstaa Since @notpritam tells that he has no experience in tests addition, may be we can merge this and i will add tests in next pull request. Anyway this feature was tested on Vercel test deployment.

Sounds okay to me 👍🏻. Thanks!

Thanks for approval, merged into master, i will open pull request with test addition soon.