-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 issues-pr-merged
and issues-pr-closed-unmerged
to [GitHubIssues]
#8708
Add issues-pr-merged
and issues-pr-closed-unmerged
to [GitHubIssues]
#8708
Conversation
|
issues-pr-merged
to [GitHub-issue-tracking]issues-pr-merged
to [GitHubIssues]
issues-pr-merged
to [GitHubIssues]issues-pr-merged
and issues-pr-closed-unmerged
to [GitHubIssues]
Yes we already have too many of these. Tbh I'd like to avoid adding more variants and I'm not inclined to merge this as it stands. As you have noticed, giving them meaningful names or routes has become difficult as the variants become increasingly esoteric. I think at this point, rather than trying to squeeze in more additional variants with their own named routes, I am more interested in thinking about how we could avoid adding more and reduce this problem. My original lofty ambition was that adding the generic issue search badge would allow us to stop adding more increasingly contrived variants of this. See #5948 https://img.shields.io/github/issues-raw/badges/shields/service-badge https://img.shields.io/github/issues-pr-closed-raw/badges/shields ..and so on. so then there are really two problems two solve to replicate the existing badges: colouring and suffixes. Suffixes is probably easier (I think?). The really naive option is to add a On the face of it, applying colours is also quite easy. All of the existing badges use the same colour scheme: So having thought it through that far, the upshot of all that was.. a) Whereas I reckon we probably can define some rules that allow us to provide a At that point, I basically never took it any further. There wasn't a totally obvious way forward and just not changing anything was the easy option. That's why #6164 kinda.. fizzled out. It isn't really "solved" but also we didn't mark it as a "good first issue" - we didn't really have an agreed implementation. So having brain-dumped all that context, I guess the question is: any thoughts on that? If we really want to add this, I'd rather put the effort into thinking about a more generic solution and what tradeoff we pick to make it work. |
Coming to this from #8664 and refreshing my memory from #6164 too. I think #8664 (comment) captures my thinking for #6164 as well; I'd rather not add more of these when the existing query badge seems perfectly sufficient |
Agreed we shouldn't try to add any more specific routes for slightly different queries. |
The existing color scheme has puzzled me in the past as well. Though I can't put my finger on it, I feel like there was a semi-recent badge addition or discussion where someone tried to replicate that behavior which brought about some questions. Issue count seems like a strictly informational data point to me, and these badges should use that coloring by default IMO. While I agree that changing the default color could cause a stir, I think there's a reasonable case to make qualifying it as a bug fix, or at least addresses a long standing inconsistency with our defaults.
Probably fair to say there's some more to my personal rationale, though in general just want to underscore that I was echoing the broader collective decision of the core team to establish a ceiling on message customization. I'm not sure what we should do for the suffix in this context, but do think it should be consistent with general policy (or at least have a strong and articulated reason if we need to deviate in this particular case) |
I do agree with you. It is definitely what we would do if we were adding this from scratch today. I just know making this change to one of our highest traffic (groups of) badges will be controversial. I think maybe step one is we just change the colour first: We replace
What do you think about my suggestion of a function getSuffix(query) {
if (query.includes("is:open")) {
return ' open'
}
if (query.includes("is:closed")) {
return ' closed'
}
return ''
} |
Maybe it was the GitLab issue/PR badges? Side note: I've just realised we have even more different variants of this for GitLab then we do for GitHub 🙀 |
I think the query param route is a fair approach. However, I think the proposed name is what's been giving me pause because I feel "withSuffix" may carry some open ended connotations. I.e. I would sympathize with a hypothetical future user that asked a question or expressed confusion because they felt like it would allow them to set, or at least have more control, over the suffix. I'll keep mulling it over but a few alternative name suggestions to consider:
|
Yeah at the moment a lot of the existing badges use
|
d'oh 🤦 My first thought was "suffixStyle" but then I convinced myself that was too long and "style" would be better 😞 |
|
What is the status of this pull request? Should we try to move it forward? :) |
Hi @PyvesB :) Unfortunately, right now I don’t have time to work on this PR - and I am not sure if a final decision has been made as to in which direction this PR should go (?) Feel free to take it over, if you have the will and capacity to do so- thanks 💙😊 |
We're somewhat short-handed in the maintainer team at the moment, unfortunately I don't think we'll be able to take this over in the near future, so I'll go ahead and close this for now. If someone else wants to pick this up, feel free to reopen the discussion and fetch the commits by running git fetch origin pull/8708/head:pr-8708. Open a new pull request with a co-authored trailer included in the commit message to give attribution to the original author. Thanks for contributing to the project @PaulaBarszcz ! 😉 |
Closes #6164
Added
issues-pr-merged
andissues-pr-closed-unmerged
to services/github/github-issues.service.jsIn my opinion we could reduce the number of badge examples related to GitHub PRs in the
issue tracking
section. We haveraw
,with prefix/suffix in the different place in the badge
,with additional by-label sorting
- for open, closed, and now also for merged and for closed unmerged PRs. Maybe we could leave as examples only:closed
+merged
put together)We can also put info about possible badge variants in the
documentation
section in the modal.It's slightly confusing that for GitHub, 'closed' means 'closed-unmerged',
while for us, 'closed' means 'closed-unmerged PLUS merged'. We probably cannot change it now because of backwards compatibility (?)
The name
issues-pr-closed-unmerged' is not very neat, but I don't have a better idea. We can't use
issues-pr-unmerged` because it suggests that all open and all closed-unmerged PRs would be counted.issues-pr-merged
:issues-pr-closed-unmerged
Previously present issues-pr-closed still correctly shows the number of MERGED issues + CLOSED issues: