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

[GitHub] Move to 'funding' category #5846

Merged
merged 4 commits into from
May 16, 2021
Merged

Conversation

kernc
Copy link
Contributor

@kernc kernc commented Nov 18, 2020

@shields-ci
Copy link

shields-ci commented Nov 18, 2020

Warnings
⚠️ This PR modified service code for github but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @kernc!

Generated by 🚫 dangerJS against 1cb334b

@kernc kernc changed the title [GitHub Sponsors] Move to 'funding' category [GitHub] Move to 'funding' category Nov 18, 2020
@calebcartwright
Copy link
Member

Thank you for the PR @kernc, but this isn't necessarily a simple change.

The badge currently defaults to the social style at the request of the community, as there was a desire to have something that approximated the look and feel of GitHub (as our Stars and Forks badges do). We also only permit social style as the default on badges that are in the social category, which is why this one was added there.

Simply modifying the category as proposed here will break that permitted default styling rule we follow, but conversely, changing the default style would impact current users of the badge (as the style for existed badges would change to the standard flat style).

While I think there's a fairly logical case that can be made for this badge being in the funding category, I don't think it's wildly out of place in the social category either. As such I'm not yet convinced that trying to retroactively change the category is worth the tradeoffs of the impacts I mentioned above, but will see if any of the other maintainers feel strongly one way or the other

@kernc
Copy link
Contributor Author

kernc commented Nov 19, 2020

I though the badge was misplaced merely because the category went unnoticed. 😅 The new badge was released exactly a month ago, and it seems to have, as of yet, very few users. But: the ones that do use it (and are searchable on GitHub), all hard-code their chosen style= as well! If there's a time to break it, it's now.

@calebcartwright
Copy link
Member

If there's a time to break it, it's now.

Thanks for looking into and sharing the results. However, we never intentionally break/modify any of our badges independent of their level of popularity, and additionally I do not think a GH search is a sufficiently complete and representative picture of badge usage.

In the last 24 hours alone we've had more than 850 requests hit our badge servers for the GH Sponsors badges (https://metrics.shields.io/d/aESRBSjmz/services?orgId=1&from=now-24h&to=now). I'm highly skeptical that all of those requests were only for those 2 repositories shown in your search, as those somehow would have had to be sequenced to avoid all the various caching layers (browsers, GH camo, CloudFlare, etc.)

I think some additional context behind the motivation would be helpful. Could expand on why you feel the category should be changed?

@kernc
Copy link
Contributor Author

kernc commented Nov 20, 2020

It shows 6+ results for me, the title saying "Showing 12 available code results". I think code search results are async-collected/cached. Maybe refresh a couple of times.

Motivation is simply moving to a better-suited category. Sponsors provide funds; followers are social. I went straight to Shields looking for this badge and only discovered it on Google via the implementing PR as it wasn't in the expected category. I'm sure you'll accrue more complaints about it in time. 😛

I agree GH search results likely don't paint a complete picture, but they form a statistical sample, and based on this sample, it's quite safe to assume a roughly proportional fraction of population equally hard-codes the style= parameter, particularly given how it's what is discharged when clicking the copy button. The badge is new and non-advertised. Promptly tended to, this will break no page. 🤚

Your call.

@calebcartwright
Copy link
Member

It shows 6+ results for me, the title saying "Showing 12 available code results". I think code search results are async-collected/cached. Maybe refresh a couple of times.

The GitHub search index results are highly variable unfortunately, so I just don't put a ton of stock into them.

image

I went straight to Shields looking for this badge and only discovered it on Google via the implementing PR as it wasn't in the expected category.

Thanks for sharing, always helpful to hear the experience folks have with the site in particular. Did you try using the Search feature (the text box at the very top of the landing page) or jump straight to the category? If there's a particular badge you are looking for it's always best to use search to find it directly, whereas I find the categories helpful for general browsing (such as learning about new analysis or build services)

image

it's quite safe to assume a roughly proportional fraction of population equally hard-codes the style= parameter, particularly given how it's what is discharged when clicking the copy button

This is based on an incorrect assumption that all users go to Shields.io and use the modal badge builder window to produce their badge urls; not everyone does, and by all accounts most do not. A lot of the Shields user base actually seems to be completely unaware of this feature 🙃

The badge is new and non-advertised

I'm not sure I know what you mean by "non-advertised". We don't "advertise" anything 😉

Promptly tended to, this will break no page. raised_back_of_hand

Your call.

I appreciate your optimism, but do not share that certainty. If we had a time machine then I think we would could've pushed harder and ended up in a different category, but we are where are, and it's now a question of tradeoffs.

Sometimes the category is really obvious (e.g. appveyor in build) but other times not so much (see keybase in social) as there could be several categories that work, or perhaps none that feel "right". As I stated before, IMO funding would probably be better for this badge, but social isn't exactly egregious either, and certainly not so out of place that it justifies potentially changing the style of existing badge users without thoughtful consideration.

I agree that the potential blast radius is not very large, but as the maintainers of the service we do still have to be mindful about impacting our users. We could certainly decide that the impact/risk is low enough and the benefits warrant making the change anyway, but would need such a breaking change to be accompanied by a transparent discussion and explicit decision. That's the driver for the followup questions and comments, so thank you for sharing your feedback and perspective!

@calebcartwright calebcartwright added needs-discussion A consensus is needed to move forward service-badge Accepted and actionable changes, features, and bugs labels Nov 21, 2020
@calebcartwright
Copy link
Member

Just dropping a note that we've not forgotten about this. We did go ahead and make the "breaking" change to remove the style override, which gives us the flexibility to change the category now without having a risk of impacting anyone.

It's spurred some background discussions on how a few other badges are grouped in categories so may take a while longer to play out and come to a decision for this one

@PyvesB
Copy link
Member

PyvesB commented May 5, 2021

In my opinion, it makes a little more sense to have this badge in the funding category.

We did go ahead and make the "breaking" change to remove the style override, which gives us the flexibility to change the category now without having a risk of impacting anyone.

In light of this, shall we just go ahead and move the badge?

@shields-cd shields-cd temporarily deployed to shields-staging-pr-5846 May 10, 2021 18:13 Inactive
@PyvesB
Copy link
Member

PyvesB commented May 15, 2021

@kernc I would be happy to move forward and get this merged, however one of the end-to-end tests is now failing. You simply need to change this line here:

cy.visit('/category/social')

Could you please take care of that?

@calebcartwright calebcartwright removed the needs-discussion A consensus is needed to move forward label May 16, 2021
@repo-ranger repo-ranger bot merged commit cc60800 into badges:master May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants