Skip to content

initial commit#220

Open
areebakwas wants to merge 1 commit intoprojectshft:masterfrom
areebakwas:master
Open

initial commit#220
areebakwas wants to merge 1 commit intoprojectshft:masterfrom
areebakwas:master

Conversation

@areebakwas
Copy link
Copy Markdown

No description provided.

Comment thread main.js
Comment on lines +13 to +14
document.getElementById('text-from-post').value = '';
document.getElementById('writer-posted').value = '';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what is the purpose of this?
Better practice is to extract to a function to help with readability and reusability

Comment thread main.js
Comment on lines +5 to +12


const textFromPost = document.getElementById('text-from-post').value;
const writerPosted = document.getElementById('writer-posted').value;

postToAdd(textFromPost, writerPosted);


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

watch for all these empty spaces

Suggested change
const textFromPost = document.getElementById('text-from-post').value;
const writerPosted = document.getElementById('writer-posted').value;
postToAdd(textFromPost, writerPosted);
const textFromPost = document.getElementById('text-from-post').value;
const writerPosted = document.getElementById('writer-posted').value;
postToAdd(textFromPost, writerPosted);

Comment thread index.html
<input type="text" class="form-control" id="text-from-post" placeholder="Post Text">
</div>
<div class="form-group">
<input type="text" class="form-control" id="writer-posted" placeholder="Your Name">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not a great id text, I would have used:

Suggested change
<input type="text" class="form-control" id="writer-posted" placeholder="Your Name">
<input type="text" class="form-control" id="post-author-input" placeholder="Your Name">

Comment thread index.html
<div id="cont-posts"></div>
<form class="module-2" id="form-post" >
<div class="form-group">
<input type="text" class="form-control" id="text-from-post" placeholder="Post Text">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the id needs to reflect the element, not the result of the value:

Suggested change
<input type="text" class="form-control" id="text-from-post" placeholder="Post Text">
<input type="text" class="form-control" id="post-text-input" placeholder="Post Text">

Comment thread main.js
messagesPost.textContent = `${post} - Posted by ${writer}`;


const withdrawMessagesHere = document.createElement('a');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bad and not clear const name. Took me a while to understand what this is doing.
Whats wrong with removeMessagesElement/container?

Comment thread main.js
withdrawMessagesHere.href = '#';
withdrawMessagesHere.textContent = 'remove';
withdrawMessagesHere.classList.add('text-danger', 'mr-2');
withdrawMessagesHere.addEventListener('click', function () {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you are using es6 with consts, but not with functions

Comment thread main.js
});

// Fxn to make post add to page html
function postToAdd(post, writer) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not arrow function, stick to es6

Comment thread main.js
});


const postSwitchFromPage = document.createElement('a');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same as above, this const name is confusing.
showHideCommentsAction or something is better.

Comment thread main.js
});


const messagesAreHere = document.createElement('div');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
const messagesAreHere = document.createElement('div');
const messagesContainer = document.createElement('div');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You call them messages, but what are they messages or posts? Be consistent!

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