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

bug fixed #53 #54

Merged
merged 1 commit into from
Mar 5, 2018
Merged

bug fixed #53 #54

merged 1 commit into from
Mar 5, 2018

Conversation

Salam-Dalloul
Copy link
Contributor

@Salam-Dalloul Salam-Dalloul commented Feb 25, 2018

Relates: #53
@claireinez @minaorangina @msmichellegar for review.
In Referance to : Curriculum planning - SPRING 2018 https://github.com/foundersandcoders/master-reference/issues/781

Copy link
Member

@minaorangina minaorangina left a comment

Choose a reason for hiding this comment

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

Almost there! Just another bug that needs taking care of.

By the way, you don't need to create a new pull request - if you push your changes to the same branch, your changes will be added to the PR automatically 😄

public/script.js Outdated
}
}
else {
console.error(JSON.parse(xhr.responseText));
Copy link
Member

Choose a reason for hiding this comment

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

Another bug :) responseText here is a normal string - not stringified JSON - so it breaks when you try to parse it.

The reason why we need to parse on line 7 is because in that case we're sending back the JSON of our blogposts. Here though, it's just a regular string!

Copy link
Member

Choose a reason for hiding this comment

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

I recommend you test this out in your own code when making this change, so you can see what I mean. For example, you could change the endpoint to something that doesn't exist, then you can see what happens when there's an error.

@claireinez
Copy link
Member

By the way, you don't need to create a new pull request - if you push your changes to the same branch, your changes will be added to the PR automatically 😄

Maybe we should close this PR and @minaorangina you could put the same review in #53 so that we have a record of everything in the same place?

@Salam-Dalloul as Mina said, you can just push to the same branch, so if you have a look at your other pull request, you can see your new changes are now in there! 😃

@Salam-Dalloul
Copy link
Contributor Author

@minaorangina all fixed ... hopefully nothing is messing

Copy link
Member

@minaorangina minaorangina 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 to me @Salam-Dalloul, thanks very much! Feel free to merge 🎉

@Salam-Dalloul
Copy link
Contributor Author

Thanks @minaorangina 😄

This pull request can be automatically merged by project collaborators

and i'm not a collaborator 😸

Copy link
Member

@claireinez claireinez left a comment

Choose a reason for hiding this comment

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

Thanks @Salam-Dalloul!

@claireinez claireinez merged commit f66bd5a into node-girls:master Mar 5, 2018
@Salam-Dalloul Salam-Dalloul deleted the fixed-bug branch March 5, 2018 12:41
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.

3 participants