2

I'm relatively new to JavaScript, so I'm not sure if I'm doing things conventionally here, of if there's a better way of doing what I'm trying to do.

I have a JavaScript function that takes about 3,600 sentences from a JSON document and inserts them automatically into my HTML code. A unique id is generated for each once in the HTML.

I want to create an onclick event for each sentence so that when it's clicked more information appears underneath about the sentence. This means I have to declare thousands of variables, one for each sentence and one for each information div associated with that sentence:

var sent1 = document.getElementById('s1');
var sent1info = document.getElementById('s1info');
var sent2 = document.getElementById('s2');
var sent2info = document.getElementById('s2info');
var sent3 = document.getElementById('s3');
var sent3info = document.getElementById('s3info');
...

This is way too much to do manually. Is there a way to automate the process of declaring these variables, or is there a better way to do what I'm doing?

For context, my intention with each variable is to feed it into this function:

sent1.onclick = function(){
    if(sent1info.className == 'open'){
        sent1info.className = 'close';
    } else{
        sent1info.className = 'close';
    }
};

From here the CSS will reduce the info box to a hight of 0 when the className is 'close' and expand it when the className is 'open'. But, again, this will require me writing out this function thousands of times.

Is there a way to do this automatically also? Or am I going about this all wrong?

Edit to show HTML:

<!DOCTYPE html>
<html>
<head>...</head>
<body>
    <div id="everything">
        <header id="theheader" class="clearfix">...</header>
        <div id="thebody" class="box clearfix">
            <aside id="page" class="side">...</aside>
            <div class="items">
                <article id="content" class="article">
                    <img id="sentpic" src="sentpic.jpg">
                    <h1>Sentences</h1>
                    <div id="sentences">
                        *** This is where the JS inserts sentences and information ***
                        <ul id='sent1' class='sentcontent'><li class='number'>1.</li><li class='thesent'>...</li></ul>
                        <div id='sent1info' class='infobox'>
                            <ul class='sentinfo'><li class='information'>Info:</li><li class='infotext'><em>...</em></li></ul>
                            <ul class='sentinfo'><li class='information'>Line:</li><li class='line'>...</li></ul>
                        </div>
                        <ul id='sent2' class='sentcontent'><li class='number'>2.</li><li class='thesent'>...</li></ul>"
                        <div id='sent2info' class='infobox'>
                            <ul class='sentinfo'><li class='information'>Info:</li><li class='infotext'><em>...</em></li></ul>
                            <ul class='sentinfo'><li class='information'>Line:</li><li class='line'>...</li></ul>
                        </div>
                        *** it goes on like this for each sent inserted ***
                    </div>
                </article>
            </div>
        </div>
        <div class="associates clearfix">...</div>
        <footer class="foot">...</footer>
    </div>
    <script type="text/javascript" src="index.js"></script>
</body>
</html>
AdeDoyle
  • 361
  • 1
  • 14
  • 3
    why not use forEach or for loop? – Josh Adams Feb 10 '19 at 18:58
  • Look into arrays.. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array – Keith Feb 10 '19 at 18:59
  • Just index the elements that have a common class to the index in the sentences array. Show how you are generating the html from the data – charlietfl Feb 10 '19 at 19:00
  • It would help to see the HTML structure that's created. Eg: are the sentence & detail in the same parent? Are they elsewhere on the page? Also, what's the function that generates this look like? If you edit your question to include those things, you'll probably get a better answer. – Bryce Howitson Feb 10 '19 at 19:11
  • Can you please show a snip of HTML that you currently use? – Roko C. Buljan Feb 10 '19 at 19:12
  • Also, do you create HTML dynamically (using JS)? – Roko C. Buljan Feb 10 '19 at 19:13
  • `onClick` takes 3 - 4 lines to set `className` to `closed` in all if/else cases. Just set it to `closed`. – radarbob Feb 10 '19 at 19:48
  • OK thanks for providing the wrapping HTML context. Now it's clear that you insert the "sentences" elements dynamically. Can you now please show the HTML for the *sentences*-dropdowns? So just provide one of the "***This is where the JS inserts sentences.***" – Roko C. Buljan Feb 10 '19 at 19:52
  • @AdeDoyle Check the answer below that shows an alternative approach for adding/removing classnames with dynamically added elements. – AndrewL64 Feb 10 '19 at 20:04
  • @RokoC.Buljan I've that added now. I should have done it in the first place, but had to dig into the JS to find. – AdeDoyle Feb 10 '19 at 20:28
  • @AdeDoyle yes nice! Thank you - Added an answer in the good hope :) – Roko C. Buljan Feb 10 '19 at 20:31

3 Answers3

2

Using HTML <details> element:

const json = [
  {thesent:"Lol", info:"This is some info 1", line:"Whatever 1..."},
  {thesent:"Lorem", info:"Some info 2", line:"Something here 2..."},
];

const template_sentence = (ob, i) => `
<details class="sentence">
  <summary>${i+1} ${ob.thesent}</summary>
  <h3>${ob.info}</h3>
  <div>${ob.line}</div>
</details>`;

document.querySelector("#sentences").innerHTML = json.map(template_sentence).join('');
<div id="sentences"></div>

Otherwise, by using your current non-semantic markup:

Targeting by ID (in your specific case) is not needed. There's other methods like the + Next Adjacent sibling selector in CSS.

And here's a JS example - should be self-explanatory, but feel free to ask.

  • Use JS to toggle a class (.active in this example) to the clickable UL element
  • Use CSS and the Next adjacent sibling selector + to make the info DIV display: block

/* Just a sample... you'll know how to modify this with the right properties I hope */
const json = [
  {thesent:"Lol", info:"This is some info 1", line:"Whatever 1..."},
  {thesent:"Lorem", info:"Some info 2", line:"Something here 2..."},
];

// The toggle function:
const toggleInfobox = ev => ev.currentTarget.classList.toggle("active");

// A single sentcontent template
const template_sentence = (ob, i) =>
`<ul class='sentcontent'>
    <li class='number'>${i+1}</li>
    <li class='thesent'>${ob.thesent}</li>
  </ul>
  <div class='infobox'>
    <ul class='sentinfo'>
      <li class='information'>Info:</li>
      <li class='infotext'><em>${ob.info}</em></li>
    </ul>
    <ul class='sentinfo'>
      <li class='information'>Line:</li>
      <li class='line'>${ob.line}</li>
    </ul>
</div>`;

// Get target element
const el_sentences = document.querySelector("#sentences");

// Loop JSON data and create HTML
el_sentences.innerHTML = json.map(template_sentence).join('');

// Assign listeners
const el_sentcontent = el_sentences.querySelectorAll(".sentcontent");
el_sentcontent.forEach(el => el.addEventListener('click', toggleInfobox));
/* BTW, why do you use <ul> ? That's not a semantic list! */
.sentcontent { padding: 0; cursor: pointer;}
.sentcontent li { display: inline-block; }

/* Arrows are cool, right? */
.sentcontent:before        { content: "\25BC"; }
.sentcontent.active:before { content: "\25B2"; }

/* Hide adjacent .infobox initially, 
/* and show adjacent .infobox on JS click */
.sentcontent        + .infobox { display: none; }
.sentcontent.active + .infobox { display: block; }
<div id="sentences"></div>

In this Stack overflow answer you can find out more about toggling an element on some button click.

Roko C. Buljan
  • 196,159
  • 39
  • 305
  • 313
  • IDs are **definitely** are not useless in general (not sure if you were implying that, but it seemed so). For certain accessibility requirements, IDs are an absolute necessity. – Andy Hoffman Feb 10 '19 at 21:19
  • @AndyHoffman I agree that IDs are useful when in need to query-parametrize anchors to allow a scroll-to like in: `//example.com#sent1` or `//example.com#sent1content` but the answers, nor the question implies such behavior - Not to talk that about `describedby` aria etc... - That's another expansion to this answer - perhaps if OP is interested in this topic... – Roko C. Buljan Feb 10 '19 at 21:24
  • Right, but you've made a general statement about IDs that, given your rank here, could spread bad advice. – Andy Hoffman Feb 10 '19 at 21:25
  • Since browsers match `CSS` selectors from right to left, this line: `.sentcontent + .infobox { display: none; }` could be optimized to `.infobox { display: none; }`. – Andy Hoffman Feb 10 '19 at 21:34
  • @AndyHoffman yeah... as a general note... but it's important to encapsulate behaviors. `.infobox` might appear elsewhere in the DOM. We don't want to `display: none;` those elements, only the ones that at some point are handled by the exact reunion on sibling selection *v:* `.sentcontent + .infobox` → `.sentcontent.active + .infobox` - Separation of concerns etc. A big topic. So no. The suggested is absolutely preferred in my opinion. – Roko C. Buljan Feb 10 '19 at 21:39
1

This question is more of an architectural issue than a need for creating dynamic variables. Consider this example:

  • ids are removed (existing class names used)
  • This pattern scales for n sentence instances
  • In handleClick, we toggle the open class on the clicked element, which lets us leverage the adjacent sibling selector via CSS
  • No need for a close class, since the absence of the open class represents the closed state.

let outerUL = document.querySelectorAll('.sentcontent')

function handleClick() {
  this.classList.toggle('open');
}

outerUL.forEach(ul => {
  ul.addEventListener('click', handleClick);
})
.sentcontent {
  cursor: pointer;
}

.sentcontent.open + .infobox {
   display: block;
}

.infobox {
  background-color: #eee;
  display: none;
  padding: .25em .5em;
}
<ul class='sentcontent'>
  <li class='number'>1.</li>
  <li class='thesent'>Sent</li>
</ul>
<div class='infobox'>
  <ul class='sentinfo'>
    <li class='information'>Info</li>
    <li class='infotext'><em>Info text</em></li>
  </ul>
  <ul class='sentinfo'>
    <li class='information'>Line info</li>
    <li class='line'>Line</li>
  </ul>
</div>

<ul class='sentcontent'>
  <li class='number'>2.</li>
  <li class='thesent'>Sent</li>
</ul>
<div class='infobox'>
  <ul class='sentinfo'>
    <li class='information'>Info</li>
    <li class='infotext'><em>Info text</em></li>
  </ul>
  <ul class='sentinfo'>
    <li class='information'>Line info</li>
    <li class='line'>Line</li>
  </ul>
</div>

https://jsfiddle.net/d91va7tq/2/

Andy Hoffman
  • 18,436
  • 4
  • 42
  • 61
  • Andy, it looks cool now! (P.S: would be nice to, instead of toggling the class to the DIV element, to toggle the class to the target/trigger element! That way we can not only control the DIV `display` using `+` operator, but also change styles to the trigger!) Good job – Roko C. Buljan Feb 10 '19 at 21:06
  • 1
    @RokoC.Buljan Good advice. Done. – Andy Hoffman Feb 10 '19 at 21:12
  • @AndyHoffman I'm having difficulty getting this to work. Do I need an onload in there somewhere? Everything is loading when I test it, but the click actually doesn't do anything. – AdeDoyle Feb 10 '19 at 22:00
  • @AdeDoyle Put the JavaScript just before the `

    ` at the bottom of the markup: ``.

    – Andy Hoffman Feb 10 '19 at 22:02
  • @AdeDoyle If this helps, here is a [full demo](https://plnkr.co/edit/oDXkSroTjO3HE2dDkKAp?p=preview) (via Plunkr) with all of your original code (plus mine). – Andy Hoffman Feb 10 '19 at 22:08
  • @AndyHoffman I have `` just before the `

    ` at the already, pointing to the file where the JS is. I tried putting it directly in just to make sure, but still no results.

    – AdeDoyle Feb 10 '19 at 22:12
  • @AdeDoyle I updated the [Plunkr demo](https://plnkr.co/edit/PxCb0cyjwdRffChOf78G?p=preview) to use the external `script` file, `index.js`. You can see I omitted the `type="text/javascript"` because it's no longer needed. – Andy Hoffman Feb 10 '19 at 22:18
  • @AdeDoyle Maybe you were adding the `` tags to the external file, which would have caused it to break. – Andy Hoffman Feb 10 '19 at 22:23
  • @AndyHoffman I think it has something to do with the fact that the external file is also being used to dynamically insert all of the sentences from the .json document. If it would be useful, I could create a branch on the plunker and upload the full html, and eg json data? – AdeDoyle Feb 10 '19 at 22:29
  • @AdeDoyle That makes sense. – Andy Hoffman Feb 10 '19 at 22:30
  • @AndyHoffman I've added a [plunker fork](https://next.plnkr.co/edit/3hPuRSPT8cmOKz3c?preview) now and it's recreating the problem I'm having. Apologies for the delay, had to do a lot of editing to cut it down to size. – AdeDoyle Feb 10 '19 at 23:52
  • @AdeDoyle I think we're good to go! Have a look [here](https://next.plnkr.co/edit/3Ml6za6gxAJO1doY). – Andy Hoffman Feb 10 '19 at 23:58
  • 1
    Working as intended with that last fix. Perfect. Just in case it's helpful to anyone else in future, I had to combine the JS from this answer into a function of its own, and call that function at the end of the function which dynamically creates the sentences, right after they've been inserted into the HTML. – AdeDoyle Feb 11 '19 at 00:18
1

When you have a very large json data then its good idee to keep in mind too not render the whole data at once, it will effect the webbrowser performance. Instead render when needed. And that is when the user click for more information.

I did some example below, make sure too read the comment

const json = [
  {thesent:"Lol", info:"This is some info 1", line:"Whatever 1..."},
  {thesent:"Lorem", info:"Some info 2", line:"Something here 2..."},
];

const container = document.querySelector(".container");
json.forEach((item)=> {
let x= item;
let el = document.createElement("li");
el.innerHTML = x.thesent;
container.appendChild(el);
el.addEventListener("click",()=> {
var infoContainer= el.querySelector(".info");
// dont create all html element at once, instead create them 
//when the user click on it. this is better when you have a very large data.
if (!infoContainer){ // not created, then create  
    infoContainer = document.createElement("div");
    infoContainer.className="info";
    var info = document.createElement("div");
    var line = document.createElement("div");
    info.innerHTML = x.info;
    line.innerHTML = x.line;
    infoContainer.appendChild(info);
    infoContainer.appendChild(line);
    el.appendChild(infoContainer);
} else if (infoContainer.style.display == "none") // created and hidden, then display it 
           infoContainer.style.display = "block";
  else infoContainer.style.display= "none"; // already displayed then hide it 
});
})
.container li >div.info >div:first-child{
font-size: 12px;

}

.container li >div.info >div:last-child{
font-size: 10px;

}
<ul class="container">

</ul>
Alen.Toma
  • 4,684
  • 2
  • 14
  • 31
  • Not exactly the markup that OP uses. He uses UL (in a strange way but yeah... :) - Nice idea about *render when needed*! – Roko C. Buljan Feb 10 '19 at 21:01
  • I was copying the way I made the nav menu in the header. I realise now I should have been using a table format (yes?), but I'm not great with JS, and this is technically functioning. I might implement this as part of the fix. – AdeDoyle Feb 10 '19 at 21:27
  • Thx @Roko C. Buljan, hade some experiences in those kind of things. and about the `markup` he need to rewrite his his anyway. So i wanted him to understand the right way or a good way of doing it. – Alen.Toma Feb 10 '19 at 21:28
  • 1
    Glad to hear it helpt you then. But you know using jquary or vue make things much simpler of rendering data. using table is not better, you cant control the design as much as you like. using divs or ul is much better but then agen its what i think. And lastly dont forget to vote :) – Alen.Toma Feb 10 '19 at 21:34
  • Unfortunately using jquery, vue, etc. isn't an option. I'm still trying to get a fix to work before I vote, but appreciate all the helpful information I'm getting all the same. – AdeDoyle Feb 10 '19 at 22:18