Fix issue with custom logo generation in badge-maker (#9644)

[zavoloklom]

• allow logo, logoPosition, logoWidth, and links as expected keys

• add validation for logo, logoPosition, logoWidth, and links keys

• add validation tests

[github-actions]

	Messages
📖	✨ Thanks for your contribution to Shields, @zavoloklom!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 6601fc0

[chris48s]

Thanks. I will have a look over this but first I really need to re-read #4947 From memory we never progressed it because there were some things we didn't really reach an agreement on. #9476 may decide one of them for us. Adding just base64 is a reasonable first step, but I want to be sure we're not doing it in a way that impacts how we would want to add named logos.

[zavoloklom]

Thanks. I will have a look over this but first I really need to re-read #4947 From memory we never progressed it because there were some things we didn't really reach an agreement on. #9476 may decide one of them for us. Adding just base64 is a reasonable first step, but I want to be sure we're not doing it in a way that impacts how we would want to add named logos.

Thank you for response!
I had a look over these discussions and I hope you'll pick a solution.
I also wonder is it appropriate to add yargs as dependency to badge-maker and setup cli to work with all options? If so - i can add commit here or create new PR.

[chris48s]

At some point in the past I looked at adopting a proper argument parsing library like yargs or commander. Because of the slightly non-standard way the arguments are currently parsed (positional argument 5 can be either style or labelColor, depending on whether it starts with an @ or not), we'd have to make a non backwards-compatible change to the CLI in order to do that.

That doesn't mean we can't do it. It would make much more sense to have flags like --labelColor blue or whatever, rather than all positional args.

Lets not load it into this PR though. This PR is a relatively small change. Re-working the CLI is going to be a bigger job, so lets not block this PR on doing that as well.

[chris48s]

We'll need to update the documentation in
https://github.com/badges/shields/tree/master/badge-maker#format
to cover the new params.

[chris48s]

I don't think there is a strong case for exposing logoPosition and logoWidth in the badge-maker package. logoPosition is not exposed at all by shields.io. logoWidth kind of is available by accident but it is not documented and we may deprecate or quietly remove it at some point.

[zavoloklom]

What do you think about adding but marking logoWidth as deprecated? I think it should be more clear.

Also I remove logoPosition from make-badge.js also as it does not used at all.

[chris48s]

As things stand right now, logoWidth is not exposed in the public API at the library level. If you call makeBadge() with a logoWidth, it will throw ValidationError: Unexpected field 'logoWidth'

I don't think we should expose logoWidth as deprecated and then remove it again later. Lets just keep this as it stands (i.e: not exposed) at the library layer.

[zavoloklom]

Ok, and what do you think about removing logoWidth from makeBadge function then as I did in this commit?

[chris48s]

I will reply to this, but I don't have time immediately

[chris48s]

Although it is not exposed via the NPM package, logoWidth is exposed via shields.io.
https://img.shields.io/badge/foo-bar-blue?logo=javascript
https://img.shields.io/badge/foo-bar-blue?logo=javascript&logoWidth=50
I would like to remove it, but just deleting the param here is not in isolation a massively helpful thing to do.

I've raised an issue about this: #10285

[PyvesB]

@zavoloklom any chance you could address Chris's feedback in the coming weeks? :)

[zavoloklom]

@zavoloklom any chance you could address Chris's feedback in the coming weeks? :)

Hi, I'll do it this week

[zavoloklom]

We'll need to update the documentation in https://github.com/badges/shields/tree/master/badge-maker#format to cover the new params.

I added description for logoBase64 and links. Skip logoWidth for now

[zavoloklom]

@chris48s Hi! I'll make all necessary changes and left comments. I'll squash my commits when you say it's ok to

[chris48s]

Don't worry about squashing or rebasing. We squash-merge PRs on this project anyway.

Just keep pushing more commits to the branch. It is easier to see what has changed between rounds of review that way.

[chris48s]

I feel like this PR adds a couple of features that are nearly ready to go but we're also going round in circles on some other things. I'm going to close this PR but I'm going to move the ideas you've raised here to the following 3 places:

1. Expose logoBase64 and links in badge-maker NPM package #10283 - in this PR, I've picked out the work you've done on exposing logos and links in the package and fixed it (see failing test case and fix). Lets see if we can get that merged quickly
2. Remove logoPosition #10284 - in this PR, I've finished the job of properly removing the redundant logoPosition` param. You're right to suggest removing it, but the implementation proposed here is incomplete.
3. remove logoWidth param #10285 - in this issue, I've proposed removing the logoWidth param, but I'll leave this one open for discussion for a bit before we do anything.

[zavoloklom]

I feel like this PR adds a couple of features that are nearly ready to go but we're also going round in circles on some other things. I'm going to close this PR but I'm going to move the ideas you've raised here to the following 3 places:

1. Expose logoBase64 and links in badge-maker NPM package #10283 - in this PR, I've picked out the work you've done on exposing logos and links in the package and fixed it (see failing test case and fix). Lets see if we can get that merged quickly
2. Remove logoPosition #10284 - in this PR, I've finished the job of properly removing the redundant logoPosition` param. You're right to suggest removing it, but the implementation proposed here is incomplete.
3. remove logoWidth param #10285 - in this issue, I've proposed removing the logoWidth param, but I'll leave this one open for discussion for a bit before we do anything.

Looking forward to see these changes!