You may have noticed that when you click on a conversation <h2> label the first time, nothing happens. On the second click, the conversation appears, and then it toggles normally. The reason for this is that you're testing:
if (x.style.display == 'none')
and although the block is invisible the first time you click the header, because you haven't set the value of the display property explicitly to 'none', the test fails. That is, it doesn't test whether the section is visible; it tests whether it has an explicit display property value of 'none', and it doesn't.
When the test fails on the first click, it does set the display property explicitly to 'none', and everything works as you'd expect after that. The issue is that the display property actually has not two states, but three: explicit 'none', explicit 'block', and not set explicitly. It starts in this third state.
You can avoid the problem by recognizing that you want to process the two types of invisibility (states 1 and 3) the same way. This means that instead of testing for state 1, as you're doing now, you can test for state 2, and let both state 1 and state 3 be the else case. Here's some sample HTML (these samples are in the Portu-WhatsApp/website/djb_samples directory of the js branch of my fork of your repo):
The CSS is just a one-liner, to hide the conversations initially:
section.conversation > div {
display: none;
}
And here's the accompanying sample JavaScript:
window.addEventListener('DOMContentLoaded', init, false);
function init() {
var triggers = document.querySelectorAll('section.conversation > h2');
for (var i = 0, len = triggers.length; i < len; i++) {
triggers[i].addEventListener('click', toggle, false);
}
}
function toggle() {
if (this.nextElementSibling.style.display == 'block') {
this.nextElementSibling.style.display = 'none';
} else {
this.nextElementSibling.style.display = 'block';
}
}
The preceding example also solves the issue you mentioned at the end of class, where you wound up writing the same function body a dozen times. Here's how it works:
The first line tells the page to listen for when the content is fully loaded, and to fire the init() function when that happens. This is a bootstrapping operation.
The init() function uses document.querySelectorAll() (you can Google for an explanation) to tell all of the <h2> elements that are children of <section class="conversation"> parents that they should listen for click events, and fire the toggle() function when a click event happens. This is also a bootstrapping function; its job is just to tell the <h2> elements to fire the toggle() function when they get clicked, and since they only need to be told that once, init() does its work at the beginning and then isn't heard from again.
The toggle() function does the toggling. When an event fires, the magic word this refers to the object where that happened, that is, to the specific <h2> element that received the click event. The next sibling element of the <h2> is the <div> we want to toggle, so we test the value of its display property. As noted above, since we want to set the value to 'block' under two different conditions (explicit 'none' and no explicit value), we run the simpler test for the one alternative condition. Now the toggling begins with the first click, and not the second.
Note that our HTML has no inline JavaScript, that is, no @onclick attributes. This is considered Good Practice, since it makes both the HTML and the JavaScript easier to develop, read, test, and modify. Our addEventListener() function has the same effect as using inline @onclick, but it's preferable for this and other reasons. But even if you keep the inline @onclick attributes (which is what I did before I learned about the addEventListener() alternative), you'll still want to change your if - then - else logic to avoid the problem of getting no response to the first click.
Here's one final design consideration: having a list of <h2> elements with nothing between them looks a little odd. How about a layout where, say, you have the list in a left sidebar and when the user clicks on an item in that list, the associated conversation is rendered. You can see an example of something like this at http://presidential.obdurodon.org/speeches.php. The disadvantage of this approach is that you can have only one conversation or speech open at a time, which might not be what you want.
You may have noticed that when you click on a conversation
<h2>label the first time, nothing happens. On the second click, the conversation appears, and then it toggles normally. The reason for this is that you're testing:and although the block is invisible the first time you click the header, because you haven't set the value of the display property explicitly to 'none', the test fails. That is, it doesn't test whether the section is visible; it tests whether it has an explicit display property value of 'none', and it doesn't.
When the test fails on the first click, it does set the display property explicitly to 'none', and everything works as you'd expect after that. The issue is that the display property actually has not two states, but three: explicit 'none', explicit 'block', and not set explicitly. It starts in this third state.
You can avoid the problem by recognizing that you want to process the two types of invisibility (states 1 and 3) the same way. This means that instead of testing for state 1, as you're doing now, you can test for state 2, and let both state 1 and state 3 be the else case. Here's some sample HTML (these samples are in the Portu-WhatsApp/website/djb_samples directory of the js branch of my fork of your repo):
The CSS is just a one-liner, to hide the conversations initially:
And here's the accompanying sample JavaScript:
The preceding example also solves the issue you mentioned at the end of class, where you wound up writing the same function body a dozen times. Here's how it works:
init()function when that happens. This is a bootstrapping operation.init()function usesdocument.querySelectorAll()(you can Google for an explanation) to tell all of the<h2>elements that are children of<section class="conversation">parents that they should listen for click events, and fire thetoggle()function when a click event happens. This is also a bootstrapping function; its job is just to tell the<h2>elements to fire thetoggle()function when they get clicked, and since they only need to be told that once,init()does its work at the beginning and then isn't heard from again.toggle()function does the toggling. When an event fires, the magic wordthisrefers to the object where that happened, that is, to the specific<h2>element that received the click event. The next sibling element of the<h2>is the<div>we want to toggle, so we test the value of its display property. As noted above, since we want to set the value to 'block' under two different conditions (explicit 'none' and no explicit value), we run the simpler test for the one alternative condition. Now the toggling begins with the first click, and not the second.Note that our HTML has no inline JavaScript, that is, no
@onclickattributes. This is considered Good Practice, since it makes both the HTML and the JavaScript easier to develop, read, test, and modify. OuraddEventListener()function has the same effect as using inline@onclick, but it's preferable for this and other reasons. But even if you keep the inline@onclickattributes (which is what I did before I learned about theaddEventListener()alternative), you'll still want to change yourif - then - elselogic to avoid the problem of getting no response to the first click.Here's one final design consideration: having a list of
<h2>elements with nothing between them looks a little odd. How about a layout where, say, you have the list in a left sidebar and when the user clicks on an item in that list, the associated conversation is rendered. You can see an example of something like this at http://presidential.obdurodon.org/speeches.php. The disadvantage of this approach is that you can have only one conversation or speech open at a time, which might not be what you want.