-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix issue with custom logo generation in badge-maker (#9644) #9645
base: master
Are you sure you want to change the base?
Conversation
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to update the documentation in
https://github.com/badges/shields/tree/master/badge-maker#format
to cover the new params.
badge-maker/index.d.ts
Outdated
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris48s so what do you think, keep it logo
or change it to logoBase64
? I personally think that logo
is better for consistence , but in future it could also work as logo
property in other places
badge-maker/index.d.ts
Outdated
logoPosition?: number | ||
logoWidth?: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@zavoloklom any chance you could address Chris's feedback in the coming weeks? :) |
Hi, I'll do it this week |
* allow `logo`, `logoPosition`, `logoWidth`, and `links` as expected keys * add validation for `logo`, `logoPosition`, `logoWidth`, and `links` keys * add validation tests
I added description for |
@chris48s Hi! I'll make all necessary changes and left comments. I'll squash my commits when you say it's ok to |
allow
logo
,logoPosition
,logoWidth
, andlinks
as expected keysadd validation for
logo
,logoPosition
,logoWidth
, andlinks
keysadd validation tests