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 #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Chitter-challenge #124

wants to merge 1 commit into from

Conversation

RedPRO16
Copy link

@RedPRO16 RedPRO16 commented Sep 5, 2022

No description provided.

})

describe('loadPosts', () => {
it('fetches notes from the server', async () => {

Choose a reason for hiding this comment

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

I don't think you need to use async functions in this case. I can understand why you would though

let samplePosts = ['Peep', 'Peep2'];

fetch.mockResponseOnce(JSON.stringify({
samplePosts

Choose a reason for hiding this comment

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

Seems pretty straightforward to include the sample posts directly rather that setting them to a variable used only once


</div>

<script type="text/javascript" src="bundle.js"></script>

Choose a reason for hiding this comment

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

Alex mentioned to us that it's good practice to include the script in the head with "Defer" in the tag to do the same thing

Copy link

@Curtis-Turk Curtis-Turk Sep 5, 2022

Choose a reason for hiding this comment

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

<script type="text/javascript" src="bundle.js" defer ></script>

Comment on lines +2 to +6
constructor(body){
this.body = body;
}
}

Choose a reason for hiding this comment

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

I guess this model wasn't finished but naturally no reason to have a class without being able to return the body

}

addPost(post){
this.posts.unshift(post);

Choose a reason for hiding this comment

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

Is there a reason you use unshift rather than push?

Choose a reason for hiding this comment

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

I'm just curious

document.querySelector('#add-new-post').addEventListener('click', () => {
const newPost = document.querySelector('#input-new-post').value;
this.displayNewPost(newPost);
document.querySelector('#input-new-post').value = "";

Choose a reason for hiding this comment

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

You could just say new post = ""

Comment on lines +55 to +59
if ('errors' in data){
this.loggedInContainer.innerText = `${data.errors.password}`
}else {
this.loggedInContainer.innerText = `${userVal} logged in`
console.log(this.user_id, this.session_key);

Choose a reason for hiding this comment

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

I appreciate this catch, nice one. I haven't seen 'errors' in data before

Comment on lines +10 to +12
let api;
let posts;
let view;

Choose a reason for hiding this comment

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

Is there a reason to why you declare them here and then give them values in the before each?

Comment on lines +38 to +44
setTimeout(() => {
try{
expect(document.querySelectorAll('div.post').length).toEqual(1);
expect(document.querySelectorAll('div.post')[0].innerText).toEqual('Test Peep');
done();
} catch(error) {
done(error);

Choose a reason for hiding this comment

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

Why are you using setTimeout here?

Comment on lines +1 to +62
h1, p, * {
font-family: sans-serif;
}

html {
/* background:#F9F7FD; */
background: linear-gradient(to top, #fff1eb 0%, #ace0f9 100%);
display: flex;
justify-content: center;
}

h1{
color: #4eb4e2;
display: flex;
justify-content: center;
}


body {
display: flexbox;
justify-content: center;
color: #607d8b;
width: 600px;
margin: 0 auto;

margin-top: 20px;
padding: 20px;
border-radius: 10px;
}

.peep {
border-bottom: 1px solid #aaa;
padding: 15px;
background: linear-gradient(to top, #fff1eb 0%, #ace0f9 100%);
border-radius: 15px;
margin-top: 10px;
margin-bottom: 10px;
}

.peep-details {
font-weight: bold;
}

.signup {
display: flex;
justify-content: center;
border-radius: 10px;
padding: 20px;
border-bottom: 1px solid #aaa;
}

.login {
display: flex;
justify-content: center;
border-radius: 10px;
padding: 10px;
border-bottom: 1px solid #aaa;
}

button {
font-size: medium;
font-weight: bold;

Choose a reason for hiding this comment

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

Great that you got to the CSS, I didn't manage to get this far

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