2
\$\begingroup\$

I have made a successful test page that satisfies my goals of using JavaScript ( jQuery specifically ) to add and remove content from a webpage. 

<!doctype html>
<head>
<title>test</title>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
</head>

<body>
<div id="main"></div>
<script>

    const foo = `<h1 id="title">Page-Foo-One</h1></div>
    <p>Some text about foo foo foo.</p>
    <h3 id="bar_link">Link to bar</h3>`;

    const bar = `<h1 id="title">Page-Bar-Two</h1></div>
    <p>Some text about bar bar bar.</p>
    <h3 id="foo_link">Link to foo</h3>`;

    $("#main").append(foo);

    $("#main").on('click','h3#bar_link', function (){
        console.log("Clicked bar_link");
        $("#main").html("");
        $("#main").append(bar);
    }).on('click','h3#foo_link', function (){
        console.log("Clicked foo_link");
        $("#main").html("");
        $("#main").append(foo);
    }).on('click','h1#title', function (){
        console.log("Clicked title");
        $("#main").html("");
        $("#main").append(foo);
    })
</script>
</body>
</html>

For this simple example where only two possible outcomes exist the code isn't too bad to look at, however I know that if this were inflated to what a full website can entail (e.g. dozens of links per page, each one putting in different content) the .on(click) chain would get very complicated.

How else could I handle this situation?

I know that I could just make two different .html files ( foo and bar ) and setup simple anchor links (i.e. <a href="...">) between them and obtain the same effect but my goal is to have an external source generate the content that is displayed.

I am getting used to JavaScript programming in general so please do point out other ways I can improve what I am doing.

\$\endgroup\$

3 Answers 3

Reset to default
1
\$\begingroup\$

Well if you use event delegation on the <h3> element you won't have to care for how many pages or links there are just bind it once on page load and it will take care of the rest assuming the links are based on h3 following your given example so that solves your main problem.

$(document).on('click', 'h3', function() {
 //do your thing
});

Since you have stated that you have an external source to generate the HTML I would create a function pageloader() to load the content.if that is going to be an ajax function to get the content or a template file using mutache.js or any other.

function pageLoader(){
    $.ajax({
        url:'path/to/content/',
        //other params...
        success:function(data){
            //load content here 
            $("#map").html(data);
        },
    });
}

But if that content was to be used in the same scenario as you have provided then you have the option of creating object literals for keeping all the pages in one place and calling them from one place and when calling use hasOwnProperty() to avoid inherited Properties.

var pages={
    pagename:function(){
     //load the content of the page
    },
    pagename2:function(){
    //load content of the page
    }
}

In this case What you need to consider is that you either keep the id of the h3 similar to the page name or define a data-attribute on the h3 and add the page name there, as we would be passing that id/data-attribute as a parameter to our pageLoader() function which holds all our pages and calls them too why because we will create the properties of the object literal as page name and then load them.

Below is a demonstration although I cant create an ajax example to show what I talked about in the first section, i will use the object literal section to create a demo since your actual concern was the binding of the links and not how to load the content, and that is covered. So you can update the pageLoader() function with whichever approach you want to load the content

function pageLoader(pagename) {
  "use strict";
  var pages = {

    "foo": function() {
      $("#main").html(`<h1 id="title">Page-Foo</h1></div>
    <p>Some text about foo foo foo.</p>
    <h3 id="foo">Link to Foo</h3><h3 id="bar">Link to bar</h3><h3 id="contact">Link to Contact</h3><h3 id="about">Link to About</h3>`);
    },
    "bar": function() {
      $("#main").html(`<h1 id="title">Page-Bar</h1></div>
    <p>Some text about conatct bla bla bla.</p>
    <h3 id="foo">Link to Foo</h3><h3 id="bar">Link to bar</h3><h3 id="contact">Link to Contact</h3><h3 id="about">Link to About</h3>`);
    },
    "contact": function() {
      $("#main").html(`<h1 id="title">Page-Contact</h1></div>
    <p>Some text about conatct bla bla bla.</p>
    <h3 id="foo">Link to Foo</h3><h3 id="bar">Link to bar</h3><h3 id="contact">Link to Contact</h3><h3 id="about">Link to About</h3>`);
    },
    "about": function() {
      $("#main").html(`<h1 id="title">Page-about-Two</h1></div>
    <p>Some text about about about about.</p>
    <h3 id="foo">Link to Foo</h3><h3 id="bar">Link to bar</h3><h3 id="contact">Link to Contact</h3><h3 id="about">Link to About</h3>`);

    }
  };

  //call the page 
  if (pages.hasOwnProperty(pagename)) {
    pages[pagename].call(this);
  }
}

//call on page load for the first time
$(document).ready(function() {
  pageLoader("foo");
});

//use event delegation and bind the h3 

$(document).on('click', 'h3', function() {
  let pagename = $(this).prop("id");
  pageLoader(pagename);
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>


<div id="main"></div>

Hope this helps

\$\endgroup\$
2
\$\begingroup\$

The code pattern:

$("#main").html("");
$("#main").append(bar);

can be simplified to:

$("#main").html(bar);

In general, the code is, clear, easy to read, and well organized. All the decisions among alternatives are reasonable and straight forward. Doing the simplest thing that might work is a good approach because it avoids adding unneeded complexity to the necessary complexity.

The use of template literals rather than strings might be unnecessary complexity because the templating features are not used. Support for template literals is not universal among browsers. The page may break for no reason for some people. To put it another way, using template literals rather than strings creates potential broken pages for no gain in browsers that support template literals.

\$\endgroup\$
1
  • \$\begingroup\$ yes, I agree on the template literals I literally didn't notice those were backticks when copied from the OP, although they are clearly multiline and should have been spotted, but those backticks can be replaced with double or single quotes and then use Mustache.js library to render the template along with the variables/placeholder to parse. But thanks for the heads up , i appreciate. \$\endgroup\$
    – Muhammad Omer Aslam
    Commented Feb 7, 2018 at 17:24
1
\$\begingroup\$

Ben is correct- the code is mostly clear and easy to read, though there is some redundancy (e.g. $("#main").append(foo);, and even just $("#main") has many occurences).

I agree with Muhammad that Event delegation is a great way to simplify the logic.

Another aspect to consider is that every time $() appears with a CSS selector (e.g. $("#main")) a DOM look-up is performed. This can be optimized by storing that DOM reference in a variable and then substituting that variable throughout the rest of the code. For more on this topic and other optimizations, see the section Cache DOM Lookups on this post. I know that post begins by bashing jQuery but if your goal is to use it then by all means keep it. However, you might consider alternatives - see youmightnotneedjquery.com.

So you could define a constant:

const mainElement = $("#main");

And then use that everywhere else:

mainElement.append(foo);

mainElement.on('click','h3#bar_link', function (){
    mainElement.html(bar); //take advice from Ben
});//... and so on ...

Additionally, the shortcut .click() could be used in place of .on('click', handler). Then to use event delegation, check the attributes of the target of the event. For example, the click handler will receive the Event object, which has the target property. The id property of that target can be fetched using .attr('id') and that could be used to determine which element was clicked and thus which action to take. Or .is() could be used to determine if the target matches a CSS selector.

const foo = `<h1 id="title">Page-Foo-One</h1></div>
    <p>Some text about foo foo foo.</p>
    <h3 id="bar_link">Link to bar</h3>`;

const bar = `<h1 id="title">Page-Bar-Two</h1></div>
    <p>Some text about bar bar bar.</p>
    <h3 id="foo_link">Link to foo</h3>`;
const mainElement = $("#main");

mainElement.append(foo);

mainElement.click(function(clickEvent) {
  const clickedId = $(clickEvent.target).attr('id');
  if (clickedId) {
    if (clickedId == 'bar_link') {
      mainElement.html(bar);
    } else {
      mainElement.html(foo);
    }
  }
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.1.1/jquery.min.js"></script>
<div id="main"></div>

I do like how Muhammad laid out the pages object in pageLoader() to map id selectors to HTML strings. However, the function calls could be removed from the mapping, and move that call to .html() to a single call later in pageLoader(), like the snippet below:

var pages = {
    "foo": `<h1 id="title">Page-Foo</h1></div>
    <p>Some text about foo foo foo.</p>
    <h3 id="foo">Link to Foo</h3><h3 id="bar">Link to bar</h3><h3 id="contact">Link to Contact</h3><h3 id="about">Link to About</h3>`,
    ...
};

if (pages.hasOwnProperty(pagename)) {
    $('#main').html(pages[pagename]);
}

That way there is only one place .html() is called instead of once per each property of the object.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ i definitely missed these 2 things you mentioned above all moving the $('#main').html() outside is way better approach than the way i used. thanks for the heads-up. it was the first ever review i submitted so am all ears to suggestions. \$\endgroup\$
    – Muhammad Omer Aslam
    Commented Feb 7, 2018 at 17:13
  • \$\begingroup\$ I have been on StackOverflow for quite some time but as I told you that this was my first review so wanted to ask that if I could update the answer according to the suggestions by referring you and Ben both about the points you raised and change the code or I cannot? we can do that on StackOverflow but don't know about code review if it is allowed or not ? \$\endgroup\$
    – Muhammad Omer Aslam
    Commented Feb 7, 2018 at 17:28
  • \$\begingroup\$ Do you mean you want to update (the code in) your answer? If so, there should be a link below it on the left side (see this)... otherwise if you mean the OPs code then know that it should not be updated, lest our answers be invalidated \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Commented Feb 7, 2018 at 17:33
  • 1
    \$\begingroup\$ Haha obviously not OP's code at least I am used to StackOverflow rules that much :D, i wanted to just know if i can update the code in my own post according to the suggestions you both gave me, thanks for informing. \$\endgroup\$
    – Muhammad Omer Aslam
    Commented Feb 7, 2018 at 17:37
  • \$\begingroup\$ just one thing more rather than removing the function call from the object literals should'nt it be better to keep it that way, because if I replace the backtick with a single quote and use the Mustach.js to render my templates and then I would have to return the HTML after rendering, because i think @Ben has a reasonable reason to replace the backticks with single or double quotes \$\endgroup\$
    – Muhammad Omer Aslam
    Commented Feb 7, 2018 at 17:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.