-
-
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 state functionality mentioned in #1011 #1012
Add state functionality mentioned in #1011 #1012
Conversation
@espadrine have you had a chance to take a look at this? I set up on a droplet the code to implement it in our dev branch https://github.com/evancohen/smart-mirror/blob/dev/README.md the current host is https://img.myrandomwebs.com/try.html if you want to check it out there. i would love to be able to shut this server and url down though after this goes live here. I was also thinking about maybe making it pull the title of the issue and have that there instead of closed or open. any thoughts on that? or perhaps if and if there is no |
2010d98
to
c2c721a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add tests for these cases? See https://github.com/badges/shields/tree/master/service-tests for details on how to write tests.
textWidth1 + 10 + data.logoWidth + data.logoPadding, | ||
textWidth2 + 10, | ||
textWidth1 + 20 + data.logoWidth + data.logoPadding, | ||
textWidth2 + 30, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to change these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was causing overlap on longer test strings from the description... if you pull it and run it with old values you'll see what i mean.
var apiUrl = githubApiUrl; | ||
var query = {}; | ||
var issuesApi = false; // Are we using the issues API instead of the repo one? | ||
if (isPR) { | ||
apiUrl += '/search/issues'; | ||
query.q = 'is:pr is:' + (isClosed? 'closed': 'open') + | ||
' repo:' + user + '/' + repo; | ||
} else if (isState && ghLabel != undefined) { | ||
apiUrl += '/repos/' + user + '/' + repo + '/issues/' + ghLabel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a template string to build this URL, to make it look a bit cleaner:
apiUrl += `/repos/${user}/${repo}/issues/${ghLabel}`;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure!
issues = data.total_count; | ||
if (isState && issuesApi) { | ||
var rightSide = isRaw ? data.state : data.title; | ||
rightSide = rightSide.length > 25 ? rightSide.substr(0,21)+'...' : rightSide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pull these out as constants (eg. const MAX_TITLE_LENGTH = 25
or something)
I feel like this should be a separate utility function, like
function truncateTitle(str) {
return str.length > MAX_TITLE_LENGTH
? str.substr(0, MAX_TITLE_LENGTH - 4) + '...'
: str;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this too...
} | ||
var rightText = isRaw? '': (isClosed? ' closed': ' open'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add one space before each ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call... i was typing fast to get this all done so we could host and add functionality
There's also a lint error: https://travis-ci.org/badges/shields/builds/243122630? Please run |
I have been super swamped with my full time gig and haven't had a chance to make revisions requested I should have time at some point next week |
If anyone would like to adopt this PR, please feel free to do so: grab the branch, add more commits addressing the comments, and open a new PR referencing this one. Will close in one month if no one has done so. |
Can we have three states instead of two?
This way we are compatible with pull requests as well. Example of pull request disguised as an issue: |
I'm so sorry about allowing this to go stale. I started a new job and between that and the fam the free time has been little to none. I do have a version of this with those changes on a pc at home that I haven't committed or pushed yet. Im currently traveling and should be able to push it when I get home. |
No problem! We're all volunteering our time, and life happens. Shields needs something similar to this for our CI (see #979) and I could work on them together. If you don't mind, I'll pick this up. |
no problem... pick it up... let me know if you need any questions or what not answered |
Will do. Thanks! |
wasn't sure the best way to implement this i added issue #1011 and have added that functionality
/github/issues-state/:owner/:repo/:number.svg will return a result such as
or depending on the current state of the issue.