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

Badge For Visual Studio Marketplace added. #1074

Merged
merged 17 commits into from
Sep 26, 2017

Conversation

ritwickdey
Copy link
Contributor

@ritwickdey ritwickdey commented Sep 11, 2017

Badge for Version
Version

Badge for Total No of Installs
Installs

Badge for Rating
rating

Issue Request #1067

@ritwickdey
Copy link
Contributor Author

Build was falling due to #979 .

server.js Outdated
{ filterType: 7, value: repo },
{ filterType: 12, value: '4096' }]
}],
flags: 914
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these values? Could you add a comment?

Are the options identical between these three badges? If so could you make a createVscodeOptions to DRY this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep identical! this is for API request. Okay I will separate it. 😃

.get('/rating/ritwickdey.LiveServer.json')
.expectJSONTypes(Joi.object().keys({
name: Joi.equal('rating', 'Rating'),
value: Joi.string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very broad, and matches e.g. the string 'undefined'. What are the expected values here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I'll add validation

@ritwickdey
Copy link
Contributor Author

ritwickdey commented Sep 23, 2017

Hi, @paulmelnikow I've separated the identical code and fixed a test case. Please review the changes.

}));

t.create('version')
t.create('version should be formatted. eg. v7.2.5')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love these.

module.exports = t;

t.create('installs should be formatted. eg. 72M')
.get('/installs/ritwickdey.LiveServer.json')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change installs to d, version to v, and rating to r for consistency? Sorry I missed this before!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. no problem.. I'll 😄

return 0; //In case required statistic is not found means ZERO.
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for refactoring this!

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Sep 24, 2017
@paulmelnikow
Copy link
Member

Thinking this through a little more, would /vscode-marketplace be a clearer URL path?

@ritwickdey
Copy link
Contributor Author

Yep, conventionally /vscode-marketplace is more cleaner route.. But I thought, it's too big route. Suggest me what you would like to prefer. 😄

[I am totally newbie, I'll do what you suggest me] 😄

@paulmelnikow
Copy link
Member

I don't think it's too long. 😄

@ritwickdey
Copy link
Contributor Author

😄 Okay. No Problem (Thanks for the suggestion).. I am now out of home. As soon as I reach to my home, I'll change all the url path. 😄

@ritwickdey
Copy link
Contributor Author

Hi @paulmelnikow, I have a question. If I want to add download badge in 3 different format (eg 7K, 7.45K or 7458). What should be routing? Should I add 3 different format?

@paulmelnikow
Copy link
Member

I'd prefer to use our standard number formatting using metric(), which provides a more consistent experience across different badges. I've made that argument in #999, e.g.

@ritwickdey
Copy link
Contributor Author

yep! I used the metric() function. Anyway. please review my code. I've changed the route accordingly and have done small refactoring. 😃

server.js Outdated
break;
}
case 'v': {
badgeData.text[0] = getLabel('Visual Studio Marketplace', data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this lower case, like we do for the other app stores? Sorry I didn't notice it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that the label 'Visual Studio Marketplace' to 'visual studio marketplace'. Right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct!

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good!

@ritwickdey
Copy link
Contributor Author

Changed to lower case 😊

@paulmelnikow paulmelnikow merged commit 3e6ba16 into badges:master Sep 26, 2017
@paulmelnikow
Copy link
Member

Thanks a bunch!

@ritwickdey
Copy link
Contributor Author

Thanks a lot to you. 😃

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.

2 participants