Skip to content
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

use shields.io directly for badge - fix #55 #103

Closed
wants to merge 1 commit into from

Conversation

balupton
Copy link
Contributor

this redirect means that the badge will always benefit from the latest shields.io rendering code, to be consistent with other badges, rather than whatever outdated version of the code we cache within our app

@balupton
Copy link
Contributor Author

would be nice to also amend this so that the query-string is also forwarded over, enabling shields.io badge customisations for free

@rauchg
Copy link
Owner

rauchg commented Nov 17, 2015

@balupton definitely interested in improving that. Only concern is the extra network hoop? Maybe we should proxy instead?

@balupton
Copy link
Contributor Author

I wouldn't consider the extra network loop to be of significance to performance.

@rauchg
Copy link
Owner

rauchg commented Nov 20, 2015

But right now we have design parity and better performance? It's hard to give up on that. Maybe shields has a module we can reuse to serve it ourselves?

@balupton
Copy link
Contributor Author

Well, I wouldn't say design parity, this fixes #55 for instance. Nor is their currently any parity with the other shields.io styling options available via an addition of query string forwarding. Meaning if the user wants to use a different style, and every other badge they use is with shields.io, the slack badge will stick out — another thing I face in the bevry and docpad projects that have been updated so far (we will eventually update all, of which there are about 200).

Also had a discussion here on shields about options: badges/shields#536

Regarding performance, I'd hardly say we have better performance, perhaps a few milliseconds... A common use case for this badge is in readmes, of which there will most likely be other hits to shields.io anyway, and I'd imagine their servers are better and perhaps have a caching thing for the specific url payload that we redirect to perhaps (this is all speculation though).

I think the ease of maintenance and automatically getting their updates is worth a few milliseconds performance degradation to be honest.

Although, if we get a shields.io module we can embed, that seems like the second best option to me.

@rauchg
Copy link
Owner

rauchg commented Nov 30, 2015

Fixed in 0.7.2.

@rauchg rauchg closed this Nov 30, 2015
@espadrine
Copy link

espadrine commented Sep 7, 2016

@rauchg Hello, shields.io author / maintainer here.

I might be reading this wrong, but it feels like you are not actually relying on the shields.io npm library (which, granted, is hard to find, thanks to complicated history). I would recommend using it in your case. See this for example code.

SVG has gone a long way since we started the projects (and given the number of bugs we raised to various browsers, I hope we helped a bit in making it easier to author it), but there are still a few pitfalls and incompatibilities that we worked on avoiding.

As a side-note, my understanding of Microsoft's Core Fonts' license is that it is not really legal to publish a copy of the Verdana font on a GitHub project (or in any public URL) without their consent.
As an aside to this side-note, I would recommend to use the TTF available on macOS by default, instead of the one available on Windows or on Linux' ttf-mscorefonts-installer package.

Alternatively, you may want to redirect to the shields.io website, by using its /badge/ endpoint, which would fix #73 for free.

Would you be interested by one of those choices? Would you consider a patch for it?

@devth
Copy link

devth commented Apr 18, 2018

Bump. I tried using style=flat-squared but it didn't seem to affect the style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants