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

feat(starred-count): show starred count #271

Merged
merged 18 commits into from
Aug 25, 2017

Conversation

adrianhartanto0
Copy link
Contributor

This is to close #129. I followed the suggestion made by @RolfKoenders, and used XMLHttpRequest instead of fetch to retrieve the star count.

I have also included a dutch and french translation for the "star" word using Google translate. @RolfKoenders @machour, could you please verify that the translated word is correct?

@adrianhartanto0 adrianhartanto0 changed the title Starred count new feat(starred-count): show starred count #271 Aug 22, 2017
Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

Quick look at the code seems good! Thanks so much!

@andrewda andrewda changed the title feat(starred-count): show starred count #271 feat(starred-count): show starred count Aug 22, 2017
src/api/index.js Outdated
const ENDPOINT = `https://api.github.com/users/${owner}/starred?per_page=1`;

const requestPromise = new Promise((resolve, reject) => {
const req = new XMLHttpRequest();
Copy link
Member

Choose a reason for hiding this comment

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

You can use fetch instead, quick example:

fetch('https://api.github.com/users/lex111/starred?per_page=1').then(function (response) {
  console.log(response.headers.get('Link'));
});

{starCount}
</Text>
<Text style={styles.unitText}>
{translate('common.star', language)}
Copy link
Member

Choose a reason for hiding this comment

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

It is better to use the plural (stars), by analogy with other (repositories, followers).

@@ -288,6 +288,7 @@ export const fr = {
},
common: {
bio: 'BIO',
star: 'étoiles',
Copy link
Member

Choose a reason for hiding this comment

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

If look at the key starsTitle, then the stars are translated as "favoris".

Copy link
Member

Choose a reason for hiding this comment

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

yes, it should be favoris. Merci beaucoup Mr @lex111 !

@@ -282,6 +284,7 @@ export const nl = {
},
common: {
bio: 'BIO',
star: 'Sterren',
Copy link
Member

Choose a reason for hiding this comment

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

And here it will be simple - Stars (again, if look at the key starsTitle)

@adrianhartanto0
Copy link
Contributor Author

5ab1671 reduce the left and right margin of the badge in user-profile-component.js to make it one line instead of multiline.

original-margin

new-margin

@@ -8,3 +8,4 @@ export const GET_FOLLOWERS = createActionSet('GET_FOLLOWERS');
export const GET_FOLLOWING = createActionSet('GET_FOLLOWING');
export const SEARCH_USER_REPOS = createActionSet('SEARCH_USER_REPOS');
export const CHANGE_FOLLOW_STATUS = createActionSet('CHANGE_FOLLOW_STATUS');
export const GET_STAR_COUNT = createActionSet('CHANGE_FOLLOW_STATUS');
Copy link
Member

Choose a reason for hiding this comment

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

Must be GET_STAR_COUNT

case GET_STAR_COUNT.PENDING:
return {
...state,
starCount: ' ',
Copy link
Member

Choose a reason for hiding this comment

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

Probably it is possible to remove, after all in initialStore already there is this key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lex111 I have decided to remove it from initialStore, and only add to it when GET_STAR_COUNT.PENDING and GET_AUTH_STAR_COUNT.PENDING are respectively dispatched.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I would still put it in initialStore, and then it could only be added to GET_STAR_COUNT.SUCCESS, but I'm ok.

@@ -282,6 +284,7 @@ export const nl = {
},
common: {
bio: 'BIO',
stars: 'Stars',
Copy link
Member

Choose a reason for hiding this comment

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

Dutch should be: 'Sterren'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your help! Will change the translation

Copy link
Member

@RolfKoenders RolfKoenders left a comment

Choose a reason for hiding this comment

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

Looks good! I added as a comment the dutch translation for 'stars'.

@housseindjirdeh
Copy link
Member

Overall this looks so solid :O :O Thanks so much @adrianhartanto0, there are literally just the few things @RolfKoenders and @lex111 noticed that I can see will need changing but otherwise this should be solid to merge 🎉

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Thanks a bunch @adrianhartanto0 🎉 🎉 🎉

@housseindjirdeh housseindjirdeh merged commit 4e9b4f2 into gitpoint:master Aug 25, 2017
chinesedfan added a commit to chinesedfan/git-point that referenced this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show starred repositories
6 participants