Skip to content
This repository was archived by the owner on Aug 17, 2024. It is now read-only.

Conversation

@rajgthub
Copy link

Here is my solution for the homework exercise. Please have a look at them. Thanks very!

public/script.js Outdated
var postText = document.createElement('p');
var thumbnail = document.createElement('img');
var postContainer = document.querySelector('.post-container');
data.map(obj => {
Copy link

Choose a reason for hiding this comment

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

obj is not a meaningful name, please go for something related to what is stored in this variable

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it should be referring to a single post. I have changed it.

public/script.js Outdated
var thumbnail = document.createElement('img');
var postContainer = document.querySelector('.post-container');
data.map(obj => {
for (var blogpost in obj) {
Copy link

Choose a reason for hiding this comment

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

you're already iterating through your array, why do you need a second loop?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is unnecessary here.

server.js Outdated
const app = express()

//reading posts
let allposts = []
Copy link

@nennes nennes Jul 17, 2018

Choose a reason for hiding this comment

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

you can combine the 2 lines in 1 and declare the variable as const

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have changed.

public/script.js Outdated
res.json().then(function(json) {
addBlogpostsToPage(json);
document.querySelector("form").reset();
window.location.href = "http://localhost:3000/posts"
Copy link

Choose a reason for hiding this comment

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

try to use a path instead of the full url. this way it would work both on your local computer and in the production server.

Copy link
Author

Choose a reason for hiding this comment

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

That is a great idea! thanks for pointing out.

public/script.js Outdated
});
})
.catch(function (err) {
console.error(err)
Copy link

Choose a reason for hiding this comment

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

in a real application you'd display some error where the user can see it.
maybe you could populate some text in a div that has error CSS styling, and make it visible.

var postContainer = document.querySelector('.post-container');
data.map(post => {
if (post.hasOwnProperty("blogpost")) {
var postDiv = document.createElement("div");
Copy link

Choose a reason for hiding this comment

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

try to use const or let instead of var, as var ofter behaves in mysterious ways

Copy link
Author

Choose a reason for hiding this comment

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

yes, I will do it. I did not realise as it was in the forked version of the code (original).

Copy link

@nennes nennes left a comment

Choose a reason for hiding this comment

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

good stuff! 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants