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

Chitter challenge #138

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

mirandaweston
Copy link

No description provided.

Copy link

@Perspicacity11 Perspicacity11 left a comment

Choose a reason for hiding this comment

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

Overall a strong expression of the brief, demonstrating a good understanding of async JS and solid test coverage. I'd probably have avoided using a Ruby backend for a JS front-end challenge but as I understand that the app works, this is a minor concern. It does seem that there is a test missing for reverse ordering of the peeps in the view, but again this is minor as it's tested in the client.test.js.

You might try to jazz up the front end with a little CSS, as you have a site that can render all the relevant information from the API; I'm not a huge aesthetic hawk though so this is splitting hairs!

const response = await fetch('https://chitter-backend-api-v2.herokuapp.com/peeps');
const data = await response.json();

return data.map(peep => ({

Choose a reason for hiding this comment

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

Well done on formatting the API response object in client before it goes into the model array, I struggled with this in my own version. The complexity of your formatted object is quite impressive.

document.body.innerHTML = fs.readFileSync('./index.html');
PeepsClient.mockClear();

const fakeClient = {loadPeeps: () => Promise.resolve(['mock peep'])};

Choose a reason for hiding this comment

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

Good use of promise resolution!

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