-
-
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
[badge/dynamic/json] fix colorscheme on error #1445
Conversation
Generated by 🚫 dangerJS |
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.
Thanks for fixing this!
service-tests/json.js
Outdated
@@ -35,7 +35,7 @@ t.create('JSON from uri | with prefix & suffix & label') | |||
|
|||
t.create('JSON from uri | object doesnt exist') | |||
.get('.json?uri=https://github.com/badges/shields/raw/master/package.json&query=$.does_not_exist') | |||
.expectJSON({ name: 'custom badge', value: '' }); | |||
.expectJSON({ name: 'custom badge', value: 'no result' }); |
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.
Want to test the color as well? @platan added a _shields_test
format which exposes it.
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.
Nice, did not realise this was a thing!
Will update the tests
switch (type){ | ||
case 'json': | ||
data = (typeof data == 'object' ? data : JSON.parse(data)); | ||
badgeData.text[1] = (prefix || '') + jp.query(data, pathExpression).join(', ') + (suffix || ''); | ||
var jsonpath = jp.query(data, pathExpression); | ||
if (!jsonpath.length) |
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 fewer surprises, would it be better to use Array.isArray
?
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.
From some testing it seems jsonpath will always return an array,
If the query finds nothing, jsonpath will just return an empty array []
examples based on the following json
{
name: 'project',
version: '1.2.3'
}
?query= |
jsonpath returned |
---|---|
$.version |
['1.2.3'] |
$.notfound |
[] |
server.js
Outdated
@@ -7606,6 +7606,7 @@ cache({ | |||
var badgeData = getBadgeData('custom badge', query); | |||
|
|||
if (!query.uri){ | |||
badgeData.colorB = '#e05d44'; // red | |||
badgeData.text[1] = 'no uri specified'; |
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.
Another option you could consider here is the setBadgeColor helper in badge-data.js. It will let you use named or hex colors interchangeably, discarding colorB if needed.
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.
That is the exact function i needed!
Did not realise it also discarded the user override.
Closes #1442
All dynamic badges are currently showing
brightgreen
on right hand side even on error.This PR causes the right hand side to be
red
if no uri specified orlightgrey
on other errors.This also adds an error if the jsonpath/
?query
returns an empty arrayno result
Notes:
Was failing due to trying to set
colorB = 'lightgrey'
instead ofcolorB = '#9f9f9f'
Also tried using
colorscheme = 'lightgrey'
but that didn't work due to the users color choice always taking priority and the user needing to specify?colorB=10ADED
for any color other than brightgreen