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

add number of comments #65

Merged
merged 1 commit into from
Oct 17, 2020
Merged

add number of comments #65

merged 1 commit into from
Oct 17, 2020

Conversation

tianlangwu
Copy link
Contributor

@tianlangwu tianlangwu commented Oct 17, 2020

This RP is for issue-57. The number of comments shows on the bottom left of each issue.
SS02

When I am run the command npm run lint:fix to check my formatting, it automatically formats something in the other files.

Let me know if I did anything incorrectly, thanks for the consideration.

Copy link
Owner

@trybick trybick left a comment

Choose a reason for hiding this comment

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

It looks really good 👍 However I am getting this error when I click the 'Get Issues' in the deploy preview, can you take a look? This might be fixed by resolving the comment about this line.

Screen Shot 2020-10-17 at 3 28 46 PM

@@ -21,7 +21,7 @@ const SearchResultsContainer = ({ currentPage, onPageChange, results }) => {
const formattedResults =
results.items[0] &&
results.items.map(item => {
const htmlUrl = item.html_url.split('/');
const { htmlUrl, comments } = [item.html_url.split('/'), item.comments];
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to do this on a separate line than the htmlUrl -
const { comments } = item;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used to do it this way,
ss03

but when I run the formating command, it said
ss04

Copy link
Owner

Choose a reason for hiding this comment

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

Oh gotcha. Please skip the lint:fix command for now

@@ -38,7 +39,8 @@ export const SearchResult = ({
<p className="body-text">{bodyText}</p>

<div className="metadata">
<div>{`${userName}/${repoName}`}</div>
<div>{`${userName}/${repoName}`} </div>
<div className="comments">{`Total comments ${comments}`}</div>
Copy link
Owner

Choose a reason for hiding this comment

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

Could we do -

Comments: ${comments}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks for the advice.

@trybick
Copy link
Owner

trybick commented Oct 17, 2020

Also sorry, there are some merge conflicts which were introduced by this PR (if it helps to look at it
#60)

@tianlangwu
Copy link
Contributor Author

tianlangwu commented Oct 17, 2020

Also sorry, there are some merge conflicts which were introduced by this PR (if it helps to look at it
#60)

I guess it is because I ran the npm run lint:fix, and it changed some format in some files.
I have fixed it, can you please check is my PR fine now? I have fixed the problem on the deploy preview too.

@trybick trybick merged commit 33ccada into trybick:master Oct 17, 2020
@trybick
Copy link
Owner

trybick commented Oct 17, 2020

Awesome, thanks @tianlangwu !

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.

2 participants