Skip to content

Eval 2 - reddit#225

Open
jlneuser wants to merge 10 commits intoprojectshft:masterfrom
jlneuser:master
Open

Eval 2 - reddit#225
jlneuser wants to merge 10 commits intoprojectshft:masterfrom
jlneuser:master

Conversation

@jlneuser
Copy link

No description provided.

Comment on lines +36 to +37
<ul class="nav nav-pills">
<li role="presentation" class="active"><a href="#">Submit</a></li>

Choose a reason for hiding this comment

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

never use ul or lists for buttons, use the button tag and you have 'btn btn-primary' bootstrap classes

Comment on lines +28 to +29
<span class="rm-comments-btn">remove</span>
<span class="show-comments-btn">comments:</span>

Choose a reason for hiding this comment

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

same, could have been buttons

<button class="btn btn-danger btn-sm">Remove</button>
<button class="btn btn-info btn-sm">Show Comments</button>

const inputName = document.getElementsByClassName('name')[0].value;

const newSpan = document.createElement('span');
newSpan.innerHTML = `${inputPost}, posted by ${inputName}`;

Choose a reason for hiding this comment

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

this is less recommented, use textContent or appendChild as they are safer and more performant.

Comment on lines +4 to +9

const inputPost = document.getElementsByClassName('post')[0].value;
const inputName = document.getElementsByClassName('name')[0].value;

const newSpan = document.createElement('span');
newSpan.innerHTML = `${inputPost}, posted by ${inputName}`;

Choose a reason for hiding this comment

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

your code alignment is off, check your lint settings. This affects readability.


toggleButtons.forEach(button => {
button.addEventListener('click', function () {

Choose a reason for hiding this comment

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

watch for too many empty spaces

})

const toggleButtons = document.querySelectorAll('.show-comments-btn');

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +30 to +31


Choose a reason for hiding this comment

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

Suggested change

margin-left: 4px;
}

/* button {

Choose a reason for hiding this comment

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

do not push commented code

@@ -0,0 +1,39 @@
const submitButton = document.getElementsByClassName('active')[0];

Choose a reason for hiding this comment

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

better to have consistency, either use getElementsByClassName or querySelector

const toggleButtons = document.querySelectorAll('.show-comments-btn');


toggleButtons.forEach(button => {

Choose a reason for hiding this comment

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

this should have been a separate function with ternary operation https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_operator

function toggleVisibility(element) {
  element.style.display = element.style.display === 'none' ? 'block' : 'none';
}

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