Fix issue with custom logo generation in badge-maker (#9644) #9645
Conversation
|
* allow `logo`, `logoPosition`, `logoWidth`, and `links` as expected keys * add validation for `logo`, `logoPosition`, `logoWidth`, and `links` keys * add validation tests
|
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! |
|
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 That doesn't mean we can't do it. It would make much more sense to have flags like 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. |
There was a problem hiding this comment.
We'll need to update the documentation in
https://github.com/badges/shields/tree/master/badge-maker#format
to cover the new params.
| @@ -4,6 +4,10 @@ interface Format { | |||
| labelColor?: string | |||
| color?: string | |||
| style?: 'plastic' | 'flat' | 'flat-square' | 'for-the-badge' | 'social' | |||
| logo?: string | |||
There was a problem hiding this comment.
At this layer, lets call this param logoBase64 instead of just logo so that it is not ambiguous with named logos when we add that. It can stay as logo at the next layer down though.
There was a problem hiding this comment.
Just thinking about this again.. in the endpoint schema, we expose logoSvg (which is literally an SVG as a string) and namedLogo. I wonder if we should just expose logoSvg here and encode it for the user to keep the package API consistent with the endpoint schema. Base64 is useful in URLs, but we don't have that constraint here. The only downside is we can't accept encoded PNG logos.
I am being quite indecisive here 🤔
| logoPosition?: number | ||
| logoWidth?: number |
There was a problem hiding this comment.
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.
| logo?: string | ||
| logoPosition?: number | ||
| logoWidth?: number | ||
| links?: Array<string> |
There was a problem hiding this comment.
Adding links is issue #4949
The reason this has been sitting around for a while is because links is implemented internally as an array for historical reasons, rather than using the label and message terminology, but we really wanted to add it as labelLink and messageLink to match the other params.
Tbh, I think I've changed my mind on this. Lets just get it done and expose it as links[] for now. If we want to change it, it is a library and we can version it. We can always deprecate this format at some later date.
| }) | ||
|
|
||
| if ('links' in format) { | ||
| if (!Array.isArray(format.links)) { |
There was a problem hiding this comment.
can we validate that the links array has a max length of 2
allow
logo,logoPosition,logoWidth, andlinksas expected keysadd validation for
logo,logoPosition,logoWidth, andlinkskeysadd validation tests