Support named logos and omit logos by default #1092
Conversation
paulmelnikow
added
frontend
|
Many features in one go.
I would merge this one immediately. |
lib/badge-data.js
Outdated
| return s !== undefined && /^(data:)([^;]+);([^,]+),(.+)$/.test(s); | ||
| } | ||
|
|
||
| function hasPrefix (s, prefix) { |
There was a problem hiding this comment.
For consistency's sake, could you avoid having a space between the function name and the opening parenthesis?
eg. function hasPrefix(s, prefix).
There was a problem hiding this comment.
Sure, will change them. I don't think debating is worth it. #1082 is a more productive way forward.
Fairly sure this is just failing due to #979: |
|
Thanks for the review! Have updated the PR. If you merge #1095 that will fix the test failure. 😄 |
|
Buh, I just closed this by accident. 😭 I'm going to go ahead and merge it into master. I've addressed the comments and the red build is due to #979. |
- Except for social badges, omit logos by default (#983) - Omit the logo from a social badge using the query string: `?logo=` (#983) - Opt in to named logos using the query string: `?logo=appveyor` - Provide custom logo data as before: `?logo=data:image/png;base64,...` - Rewrite badge data functions, with unit tests Unit tests are covering the new code very well, though the underlying functionality (logos) is untested. Close #983
|
Shouldn't appveyor lose it's logo with this now? Also, I can't find any information/details about that new behavior or named logo explanation etc. on the shields webpge. |
|
Right, there should be some explanation on the homepage. I'll get a pull request open with that. As far as appveyor, the logo is still there because this change hasn't been deployed yet. |
|
Nice! Just curious what logos are supported with these changes |
|
Here's the list as of what is currently deployed: https://github.com/badges/shields/tree/gh-pages/logo |
|
I've lost the thread - is there a PR that has been committed instead of this one? I'd like to see how logo removal is implemented. |
|
@techtonik think it is #1092 |
|
@RedSparr0w it is #1092 already. =) |
|
@paulmelnikow Thanks for that! If I want to contribute an svg, are there any requirements? (single color, etc) |
|
@tylerl0706 To stay on topic, let's discuss that in #1107. |
|
Requiring people to opt-in for logos by duplicating badge names looks crude - |
|
I understand that, for a clean and more uniform look of all badges, it defaults to off. The badge link basically contains its reference for a service logo already.
I'm pretty sure there is some pattern. :) |
|
Nice idea. Or |
|
I am still not convinced on defaults without logo - https://shields.io/ - all badges look the same without it. |
?logo=(Support logo= to turn off the badge #983)?logo=appveyor?logo=data:image/png;base64,...Unit tests are covering the new code very well, though the underlying functionality (logos) is untested.