I have a navigation menu that displays a group of notes from the clicked topic. The note can either have class note
if it is visible or class invisible
if it is hidden. I want only the notes from the topic clicked to show.
The problem is that some notes from other topics are also being shown. Even though the length of thisTopic
is always 2.
I am new to JavaScript so maybe there is an error in my loop? Thanks in advance 🙂
function openTopic(evt, topicName) {
var allNotes, thisTopic;
/* Hide all notes */
allNotes = document.getElementsByClassName("note");
for (i = 0; i < allNotes.length; i++) {
allNotes[i].classList.add("invisible");
allNotes[i].classList.remove("note");
}
/* Show notes with correct topic */
thisTopic = document.getElementsByClassName(topicName);
for (i = 0; i < thisTopic.length; i++) {
thisTopic[i].classList.add("note");
thisTopic[i].classList.remove("invisible");
}
}
.box {
border-radius: 10px;
box-shadow: 5px 5px 8px #999;
margin: 10px;
padding: 10px;
}
.note {
display: block;
background-color: #ddd;
}
.invisible {
display: none;
}
<nav class='box'>
<h3>Navigation</h3>
<ul>
<li onClick="openTopic(event, 'topic1')">Topic 1</li>
<li onClick="openTopic(event, 'topic2')">Topic 2</li>
<li onClick="openTopic(event, 'topic3')">Topic 3</li>
</ul>
</nav>
<main>
<section class='note topic1 box'>
<p>First topic 1 note</p>
</section>
<section class='note topic1 box'>
<p>Second topic 1 note</p>
</section>
<section class='note topic2 box'>
<p>First topic 2 note</p>
</section>
<section class='note topic2 box'>
<p>Second topic 2 note</p>
</section>
<section class='note topic3 box'>
<p>First topic 3 note</p>
</section>
<section class='note topic3 box'>
<p>Second topic 3 note</p>
</section>
</main>
Advertisement
Answer
There are two issues with your code:
- You’re removing the
note
class from elements. - You’re not giving elements that should be hidden by default the
invisible
class.
function openTopic(evt, topicName) {
var allNotes, thisTopic;
/* Hide all notes */
allNotes = document.getElementsByClassName("note");
for (i = 0; i < allNotes.length; i++)
allNotes[i].classList.add("invisible");
/* Show notes with correct topic */
thisTopic = document.getElementsByClassName(topicName);
for (i = 0; i < thisTopic.length; i++)
thisTopic[i].classList.remove("invisible");
}
.box {
border-radius: 10px;
box-shadow: 5px 5px 8px #999;
margin: 10px;
padding: 10px;
}
.note {
display: block;
background-color: #ddd;
}
.note.invisible {
display: none;
}
<nav class='box'>
<h3>Navigation</h3>
<ul>
<li onClick="openTopic(event, 'topic1')">Topic 1</li>
<li onClick="openTopic(event, 'topic2')">Topic 2</li>
<li onClick="openTopic(event, 'topic3')">Topic 3</li>
</ul>
</nav>
<main>
<section class='note topic1 box'>
<p>First topic 1 note</p>
</section>
<section class='note topic1 box'>
<p>Second topic 1 note</p>
</section>
<section class='note topic2 box invisible'>
<p>First topic 2 note</p>
</section>
<section class='note topic2 box invisible'>
<p>Second topic 2 note</p>
</section>
<section class='note topic3 box invisible'>
<p>First topic 3 note</p>
</section>
<section class='note topic3 box invisible'>
<p>Second topic 3 note</p>
</section>
</main>
With that in mind however, I’d strongly recommend a few things:
- Reverse your logic. (Hide by default, then activate.)
- Use the
id
attribute at a higher level instead ofclass
fortopic#
.
Reverse Your Logic
Currently, you have 3 topics with 2 notes each. Imagine instead, that you have 5 topics with 5 notes each. With your current logic, you’ll need to assign the invisible
class, by default to 20 section
elements. Instead, use an active
class and you’ll only need to assign it to 5 section
elements:
.note {
display: none;
background-color: #ddd;
}
.note.active { display: block }
You can see how this impacts your snippet below when taken to a larger scale:
function openTopic(evt, topicName) {
var allNotes, thisTopic;
/* Hide all notes */
allNotes = document.getElementsByClassName("note");
for (i = 0; i < allNotes.length; i++)
allNotes[i].classList.remove("active");
/* Show notes with correct topic */
thisTopic = document.getElementsByClassName(topicName);
for (i = 0; i < thisTopic.length; i++)
thisTopic[i].classList.add("active");
}
.box {
border-radius: 10px;
box-shadow: 5px 5px 8px #999;
margin: 10px;
padding: 10px;
}
.note {
display: none;
background-color: #ddd;
}
.note.active {
display: block;
}
<nav class='box'>
<h3>Navigation</h3>
<ul>
<li onClick="openTopic(event, 'topic1')">Topic 1</li>
<li onClick="openTopic(event, 'topic2')">Topic 2</li>
<li onClick="openTopic(event, 'topic3')">Topic 3</li>
</ul>
</nav>
<main>
<section class='note topic1 box active'>
<p>Topic 1 Note 1</p>
</section>
<section class='note topic1 box active'>
<p>Topic 1 Note 2</p>
</section>
<section class='note topic1 box active'>
<p>Topic 1 Note 3</p>
</section>
<section class='note topic1 box active'>
<p>Topic 1 Note 4</p>
</section>
<section class='note topic1 box active'>
<p>Topic 1 Note 5</p>
</section>
<section class='note topic2 box'>
<p>Topic 2 Note 1</p>
</section>
<section class='note topic2 box'>
<p>Topic 2 Note 2</p>
</section>
<section class='note topic2 box'>
<p>Topic 2 Note 3</p>
</section>
<section class='note topic2 box'>
<p>Topic 2 Note 4</p>
</section>
<section class='note topic2 box'>
<p>Topic 2 Note 5</p>
</section>
<section class='note topic3 box'>
<p>Topic 3 Note 1</p>
</section>
<section class='note topic3 box'>
<p>Topic 3 Note 2</p>
</section>
<section class='note topic3 box'>
<p>Topic 3 Note 3</p>
</section>
<section class='note topic3 box'>
<p>Topic 3 Note 4</p>
</section>
<section class='note topic3 box'>
<p>Topic 3 Note 5</p>
</section>
</main>
Use id
and grouping
It’s common practice to group elements within a parent element. In your case, I recommend encapsulating your section
elements into a div
that is designed for each topic using the id
attribute:
<div id="topic1Notes" class="topic-container active">
<section class="note box">
<p>Topic 1 Note 1</p>
</section>
</div>
Use Bootstrap
If using third party tools like Bootstrap aren’t out of the question, they already handle this and provide very easy to follow code for doing so, taking the leg work off of you:
.box {
border-radius: 10px;
box-shadow: 5px 5px 8px #999;
margin: 10px;
padding: 10px;
}
.note {
display: block;
background-color: #ddd;
}
.note.invisible {
display: none;
}
<script src="https://cdn.jsdelivr.net/npm/bootstrap@5.0.0-beta2/dist/js/bootstrap.bundle.min.js"></script>
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.0-beta2/dist/css/bootstrap.min.css" rel="stylesheet" />
<nav>
<div class="nav nav-tabs" id="nav-tab" role="tablist">
<button class="nav-link active" id="nav-home-tab" data-bs-toggle="tab" data-bs-target="#nav-home" type="button" role="tab" aria-controls="nav-home" aria-selected="true">Topic 1</button>
<button class="nav-link" id="nav-profile-tab" data-bs-toggle="tab" data-bs-target="#nav-profile" type="button" role="tab" aria-controls="nav-profile" aria-selected="false">Topic 2</button>
<button class="nav-link" id="nav-contact-tab" data-bs-toggle="tab" data-bs-target="#nav-contact" type="button" role="tab" aria-controls="nav-contact" aria-selected="false">Topic 3</button>
</div>
</nav>
<div class="tab-content" id="nav-tabContent">
<div class="tab-pane fade show active" id="nav-home" role="tabpanel" aria-labelledby="nav-home-tab">
<section for="topic1" class='note topic1 box'>
<p>First topic 1 note</p>
</section>
<section class='note topic1 box'>
<p>Second topic 1 note</p>
</section>
</div>
<div class="tab-pane fade" id="nav-profile" role="tabpanel" aria-labelledby="nav-profile-tab">
<section class='note topic2 box'>
<p>First topic 2 note</p>
</section>
<section class='note topic2 box'>
<p>Second topic 2 note</p>
</section>
</div>
<div class="tab-pane fade" id="nav-contact" role="tabpanel" aria-labelledby="nav-contact-tab">
<section class='note topic3 box'>
<p>First topic 3 note</p>
</section>
<section class='note topic3 box'>
<p>Second topic 3 note</p>
</section>
</div>
</div>
Best of luck!