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

No description provided.

Comment on lines +13 to +14
document.getElementById('text-from-post').value = '';
document.getElementById('writer-posted').value = '';

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 on lines +5 to +12


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

postToAdd(textFromPost, writerPosted);


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);

<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">

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">

<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">

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">

messagesPost.textContent = `${post} - Posted by ${writer}`;


const withdrawMessagesHere = document.createElement('a');

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?

withdrawMessagesHere.href = '#';
withdrawMessagesHere.textContent = 'remove';
withdrawMessagesHere.classList.add('text-danger', 'mr-2');
withdrawMessagesHere.addEventListener('click', function () {

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

});

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

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

});


const postSwitchFromPage = document.createElement('a');

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.

});


const messagesAreHere = document.createElement('div');

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');

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

Comments