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

Add links for [Github] Activity Badges #6096

Closed
wants to merge 6 commits into from
Closed

Conversation

rajat19
Copy link

@rajat19 rajat19 commented Jan 22, 2021

Part of solving #2271

Currently Covers all badges included in activity section which includes following

Badge (Type: Activity) Badge path Link (example for badges/shields)
All Contributors /github/all-contributors https://github.com/badges/shields/graphs/contributors
Commit Activity /github/commit-activity https://github.com/badges/shields/graphs/commit-activity
Commit Since Tagged Version /github/commit-since a0663d8...HEAD
Commit Since Tagged Version (Branch) /github/commit-since a0663d8...historical
Commit Since Latest Release (By Date) /github/commit-since latest...HEAD
Commit Since Latest Release (By Date) for a branch /github/commit-since latest...historical
Commit Since Latest Release (By Date including pre-releases) /github/commit-since latest...HEAD
Commit Since Latest Release (By SemVer) /github/commit-since latest...HEAD
Commit Since Latest Release (By SemVer incl. pre-releases) /github/commit-since latest...HEAD
Contributors /github/contributors https://github.com/badges/shields/graphs/contributors
Last Commit /github/last-commit https://github.com/badges/shields/commits/master
Last Commit (Branch) /github/last-commit https://github.com/badges/shields/commits/gh-pages
Release Date /github/release-date https://github.com/badges/shields/releases
Pre-release Date /github/release-date-pre https://github.com/badges/shields/releases

Got issue #6042 while working on this

@shields-ci
Copy link

Messages
📖 ✨ Thanks for your contribution to Shields, @rajat19!

Generated by 🚫 dangerJS against c72bd50

@calebcartwright calebcartwright added the service-badge New or updated service badge label Jan 23, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6096 January 23, 2021 08:19 Inactive
@calebcartwright
Copy link
Member

Several of these badges are not in the social category, and as I recall we only permit (by practice) setting links for badges in the social category because the social badge renderer is the only one that will actually do it.

@paulmelnikow @chris48s - you're both far more familiar with the renderers than I, any concerns/issues/etc. with adding default links to non-social badges in the service class definitions?

@rajat19
Copy link
Author

rajat19 commented Jan 24, 2021

@calebcartwright I have asked the same question on the issue #2271
15 days back but didn't get any reply on that
Because of which I raised PR as it is

@chris48s
Copy link
Member

Yeah we have tended to only do this for social badges, but we say in the guidelines

badges can link to a third-party website providing more information, either related to the metadata provided by the badge or about the project

https://github.com/badges/shields/blob/master/spec/SPECIFICATION.md#guidelines

I did a quick search and we do currently have two non-social badges that include a link by default:

There's no issue with rendering a link on a non-social badge per-se (e.g: https://img.shields.io/dependabot/semver/bundler/puma ) but it is an uncommonly used/not very useful feature because almost everyone embeds badges with an <img> tag not an <object> tag. I guess that's my concern with adding links by default to loads of other badges. It is a bit of an unfortunate feature in that respect because it seems like it would be useful but doesn't work how most users expect it too and generally creates confusion rather than utility. That said, some really high traffic badges use it so I think we're stuck with it forever.

Even if we do add link attrs by default to a bunch of other badges, I think I'm not in favour of allowing link in the frontend previews on the home page, otherwise the frontpage of https://shields.io/ turns into a really random bag of links out to other disparate stuff.

Sorry. That's not really a decision..

@calebcartwright
Copy link
Member

otherwise the frontpage of https://shields.io/ turns into a really random bag of links out to other disparate stuff.

Is this in reference to the badge listings?

@chris48s
Copy link
Member

Is this in reference to the badge listings?

Basically, I wouldn't like to see all of these badges be links out to external websites:

Screenshot at 2021-01-24 19-46-23

@calebcartwright
Copy link
Member

Basically, I wouldn't like to see all of these badges be links out to external websites:

I've got some concerns about plugging in all these links too, though aren't all the badge listings configured to just pop up the badge builder? I didn't think any of them would navigate out to an external site instead of launching the builder window (but if they would I definitely share your concern)

I think my two main concerns are:

the side effects of #6042 (I could've sworn we had an existing issue for this, but can't find one. Maybe because it's just a well-known behavior and considered more of an expectation than a bug). If we move to a position where we have entirely separate (or even somewhat disjointed) examples and render functions, as would be required today to move forward with these changes, then I feel like we're going to greatly increase the surface for potential drift between the badge listings and the actual badges, and/or add annoying little maintenance paper cuts (since tweaks to the actual rendering logic would no longer necessarily be automatically reflected in the listings)

and this 👇 especially since markdown/GitHub are where are badges used most, and the embedded links just don't work

but it is an uncommonly used/not very useful feature because almost everyone embeds badges with an tag not an tag. I guess that's my concern with adding links by default to loads of other badges. It is a bit of an unfortunate feature in that respect because it seems like it would be useful but doesn't work how most users expect it too >

@chris48s
Copy link
Member

I didn't think any of them would navigate out to an external site instead of launching the builder window

Yes - they currently launch the builder modal. I'm just saying whatever we do re #6042 should not change that - they should continue to launch the builder modal.

@calebcartwright
Copy link
Member

I'm just saying whatever we do re #6042 should not change that - they should continue to launch the builder modal.

Gotcha, and agreed 👍

@calebcartwright calebcartwright changed the title Add links for Github Activity Badges Add links for [Github] Activity Badges Jan 26, 2021
@rajat19
Copy link
Author

rajat19 commented Jan 26, 2021

I totally agree that static previews should not have links in them.

@chris48s @calebcartwright what's your final decision regarding adding links to badges other than the ones present in social template (not in static previews but otherwise)?
should i continue working on this ? or should i add just the social ones and reject this PR ?

@calebcartwright
Copy link
Member

@chris48s @calebcartwright what's your final decision regarding adding links to badges other than the ones present in social template (not in static previews but otherwise)?

Somewhat related aside - I updated the PR title to follow our conventions so that the associated tests would be run, and these changes definitely seem to be causing some non-specious test failures. We'd definitely need to have CI passing for any of the changes to be potentially mergeable.

As for the changes themselves.. I don't think we'll be able to proceed in the immediately foreseeable future, but that doesn't necessarily mean we won't ever accept some or even all of these changes. There's been some potential concerns raised, and there's also a few decisions that we'll need to make first given the potential impacts those decisions could have here.

For the time being I think folks that want clickable badges would want to follow the current recommendation of utilizing the corresponding markdown/markup/etc. syntax based on the context in which they're using the badges.

@chris48s
Copy link
Member

Does anyone else from the core team have a view on this one? @paulmelnikow ? @PyvesB ?

@PyvesB
Copy link
Member

PyvesB commented Jan 27, 2021

For the time being I think folks that want clickable badges would want to follow the current recommendation of utilizing the corresponding markdown/markup/etc. syntax based on the context in which they're using the badges.

I would tend to agree with this statement. It would be nice to convey this on the documentation of link on the homepage.

As a side note, I'm a bit annoyed that we've ended with somewhat inconsistent practices. Is there really no good reason why we've been adding default links to social badges? I'm guessing we probably have to live with it at this point, but we could add a note to our guidelines and remove default links from the two deviant non-social badges.

@chris48s
Copy link
Member

The twitter and github fork/star/watch badges have had default links for a really really long time (since 2015) and are quite widely used which kind of set that precedent for default links on social badges.

@PyvesB
Copy link
Member

PyvesB commented Mar 14, 2021

I updated our contributing guidelines a few weeks ago to clarify that only badges which have a default style equal to social should have links on by default. Accordingly, I'm going to go ahead and close this.

Thanks for opening this @rajat19, even if it didn't get merged, it ultimately resulted in a useful discussion and an improvement of our guidelines. 👍🏻

@PyvesB PyvesB closed this Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants