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

Reimagining the static badge URLs #2673

Open
paulmelnikow opened this issue Jan 7, 2019 · 13 comments
Open

Reimagining the static badge URLs #2673

paulmelnikow opened this issue Jan 7, 2019 · 13 comments
Labels
core Server, BaseService, GitHub auth, Shared helpers

Comments

@paulmelnikow
Copy link
Member

The static badge URL is well established, yet quirky. It uses a nonstandard escaping practice which only exists in Shields. It's inconvenient to generate programmatically for just that reason.

@RedSparr0w made an effort a while back to create a new one using the query string: #1395. I think it's a nice idea to do that and that we should do that using the schema for the endpoint badge, once that goes live #2495. That will support well the cases where badge URLs need to be generated in code.

Someone on Discord today was asking about a url like https://img.shields.io/badge/PyCalVer-v201901.0019--beta-blue.svg where the version sometimes has a suffix and sometimes doesn't, and thus sometimes but not always requires escaping. I'm checking if that is a programmatic case or a manual case.

We could also consider adding a version that uses the more traditional / separator, e.g. /badge/:label?/:message/:color? My only concern is about how to include slashes in the label and message. I'm not sure how to solve that. Maybe that case could fall back to either the query-string version or the legacy version.

Obviously we'll need to support the legacy version as well.

@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Jan 7, 2019
@paulmelnikow
Copy link
Member Author

Also ref #815

@mbarkhau
Copy link
Contributor

mbarkhau commented Feb 16, 2019

I've done some very basic experiments for this and have some initial suggestions. It looks like we can piggy back off the work done on the endpoint badge.

Much of these suggestions here are copied from the documentation of the endpoint page and the main page. It probably makes sense for the parameters of static badge to match what exists as closely as possible.

The url might be https://img.shields.io/badge/static.(svg|png|gif).

Available query string parameters:

property required default example
label yes &label=My%20Label
message yes &message=My%20Message
color lightgrey &color=green
labelColor grey &labelColor=9cf
isError false &isError=true
namedLogo none &namedLogo=npm
logoColor none &logoColor=red
logoWidth none &logoWidth=40
logoPosition none &logoPosition=
style flat &style=for-the-badge

Some parameters that may need further discussion.

  • schemaVersion: I'm not sure what the intent of this is or if it makes sense in the context of the static badge.
  • logoSvg: It probably doesn't make sense to send svg images in a query string. Though I just saw that the dynamic badge supports &logo=image/png... so maybe I'm wrong on this.
  • cacheSeconds: It seems like infinate is the appropriate value for the static badge. I think the query string fully defines the rendering which shouldn't change. Perhaps there is some aspect of rendering that might change over time? For example, the addition of a new namedLogo could be an issue if caches are not invalidated when it's deployed.

@paulmelnikow
Copy link
Member Author

Hey there! Thanks for picking this up 😄

Putting schemaVersion in there makes it possible to make changes in the future without breaking anyone’s badges. I like the idea of doing that, though it’s awfully wordy to say ?schemaVersion=1 in a query parameter. How about we put the version in the URL instead, like /static/v1.svg?

Along those lines I’d suggest we make it just /static/v1.svg, not /badge/static/v1.svg. People seem to be in favor of this change for the endpoint badge and I think the less typing – and the less visual noise in readmes – the better.

In terms of the colors, there’s been adequate discussion about syncing up the existing colorA and colorB parameters with the names labelColor and color which are used elsewhere. Adding a new static badge with support for labelColor and color query parameters would be okay, though it would also be okay and possibly a little clearer to first make them work globally (via coalesceBadge).

In terms of logos: yea, we do support custom SVG logos on all the badges. IMO it would be good to update the globally supported logo parameters to match the endpoint badge along the same lines, with ?logo= supported for backward compatibility. Are others folks good with that?

Agreed that cacheSeconds doesn’t make sense for the static badge. That’s an interesting point about new named logos. There’s some cache logic that triggers re-renders after server restarts, so hopefully that would keep the stale time down. I’m open to revisiting that logic, though it could be done separately from the rest of this. (Also: same argument on renaming maxAge to cacheSeconds.)

I’m sort of on the fence about whether isError should be supported, though I guess I don’t have a problem with it.

I do like the idea of being able to stringify the endpoint object into a query string and have that work with the static badge URL. That could be useful for building a UI to test custom badge endpoints, among other things.

@mbarkhau
Copy link
Contributor

How about we put the version in the URL instead, like /static/v1.svg?

Works for me 👍

though it would also be okay and possibly a little clearer to first make them work globally (via coalesceBadge).

Sounds good. Would it be appropriate to make that change as part of this ticket?

@paulmelnikow
Copy link
Member Author

We can discuss it in this issue though I think it’s better done in two PRs. We like small PRs because they are easier to review, faster to merge, and less likely to conflict with other work.

@mbarkhau
Copy link
Contributor

mbarkhau commented Feb 17, 2019

I think I didn't understand the purpose of isError originally. I guess the idea is to signal from the endpoint that an error happened. If that's the case, then I guess it doesn't make much sense in the case of a static endpoint.

@mbarkhau
Copy link
Contributor

A question about the internal naming. Since there is so much that already uses "static" in the name, which refers to the current static badge, perhaps "query string" is more appropriate for the internal naming. For example, the service class might be called QueryStringBadge in query-string/query-string.service.js.

@tomj
Copy link

tomj commented Feb 18, 2019

Note that if you stumble into this issue looking for correct way to escape / encode URL's for Shields.IO badges then as @RedSparr0w just pointed out in the Shields.IO Discord the following needs to be applied to the label and message values:

function encodeField(s) {
  return encodeURIComponent(s.replace(/-/g, '--').replace(/_/g, '__'))
}

@tomj
Copy link

tomj commented Feb 18, 2019

Also I can devote some time helping with grunt work on this - I'm not a super great Python dev (Swift is my bag 😀) but happy to help where I can, I have a use case for nice url's cc @mbarkhau @paulmelnikow

@paulmelnikow
Copy link
Member Author

A question about the internal naming. Since there is so much that already uses "static" in the name, which refers to the current static badge, perhaps "query string" is more appropriate for the internal naming. For example, the service class might be called QueryStringBadge in query-string/query-string.service.js.

It's a nice thought. In documentation I don't think we should give it a new name because it's just another way of accessing the same functionality. Internally if we need to distinguish it from the existing StaticBadge we could call it the QueryStringStaticBadge though I think it would be good to keep it with the other static badge code in e.g. services/static-badge/query-string-static-badge.service.js.

@tomj Happy to have your help! It sounds like @mbarkhau is working on the query-param version. Do you want to think a bit about a new format for the URL version? I've been thinking a bit more about this and there may not be a URL form that can be parsed with path-to-regexp that doesn't require escaping for special characters. Though perhaps it's not necessary to provide a way to escape the slashed version if we have the legacy one with dashes and the query param version.

@tomj
Copy link

tomj commented Feb 18, 2019

no worries! I'll add some basic questions to my todo list so we're all on the same page here and have some base requirements hammered out :)

paulmelnikow pushed a commit that referenced this issue Feb 20, 2019
This makes it possible to override the parameters `color` and `labelColor`.

ref #2673
paulmelnikow pushed a commit that referenced this issue Feb 22, 2019
@paulmelnikow
Copy link
Member Author

Query string static service is live! Thanks so much for your work @mbarkhau!

@paulmelnikow
Copy link
Member Author

As we discussed I'll leave this issue open to tackle this:

We could also consider adding a version that uses the more traditional / separator, e.g. /badge/:label?/:message/:color? My only concern is about how to include slashes in the label and message. I'm not sure how to solve that. Maybe that case could fall back to either the query-string version or the legacy version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

No branches or pull requests

3 participants