0
\$\begingroup\$

This code (http://jsbin.com/isexez/5/edit) functions as expected: (1) it displays the page selected; and (2) it highlights the number of the selected page.

I would appreciate help in improving it.

I am in the process of writing an ebook (http://www.keepcount.net/ebook/01-09/02_tricks.html) and need to improve this code before I get back to my writing.

* Here's my jQuery code as suggested by @Gisborne *
See also jsbin.com/iduxol/2/edit

<!DOCTYPE html>
<html>
<head>
<!-- Need to add http: to link in following line -->
<script class="jsbin" src="//ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js"></script> -->

<meta charset=utf-8 />
<title>JS Bin</title>

<style>
.nodisp {  display:none;  } 
.highlight { background:yellow; }
</style>

</head>

<body>

<!-- When page numbers (p1 & p2) are clicked, page's text is displayed and page number remains highlighted -->
<span class="pageNumbers">
<a id="1"> p1 </a>
<a id="2"> p2 </a>
</span>

<!-- Show page 1 initially -->  
<div id="pg1" class="page">
   text on page 1
</div>

<!-- Hide page 2 initially  -->
<div id="pg2" class="page nodisp">
   quiz on page 2
</div>

<script>
  $("span.pageNumbers a").click(function () {

  // Remove all highlighting
    $("span.pageNumbers a").removeClass("highlight");

  // Add highlighting to clicked page number
    $(this).addClass("highlight");

  // Hide all text
    $("div.page").hide();

  // Show text corresponding to clicked page number
    $("#pg"+ $(this).attr('id')).show();

  });

</script>
</body>
</html>
\$\endgroup\$
0

2 Answers 2

Reset to default
1
\$\begingroup\$

I came up with several changes, however most of these require the latest jQuery as well as making use of HTML5. Here's why.

First of all, in HTML5 you have a data attribute that is easily accessible via jQuery .data(). This means you can store massive amounts of element related data on each specific element and recall it as needed. Which is why I rewrote your HTML as follow:

HTML 

<!-- When page numbers (p1 & p2) are clicked, page's text is displayed and page number remains highlighted.
     Here I replaced the id attribute with a data attribute that contains each pages exact id
     You may also noticed the change in ID's simply to help better represent what pg they correlate too.  
     This will be used later to make dynamic call on load to start things off on the right foot -->
<span class="pageNumbers">
    <a id="a-1" data-page="#pg1" href="javascript:void(0)"> p1 </a>
    <a id="a-2" data-page="#pg2" href="javascript:void(0)"> p2 </a>
</span>

<!-- Show page 1 initially -->  
<div id="pg1" class="page">
   text on page 1
</div>

<!-- Hide page 2 initially  -->
<div id="pg2" class="page nodisp">
   quiz on page 2
</div>

Secondly, one of the main purposes of jQuery is to "Do more, write less", thus they have many different ways to initiate an action, but the idea is to find the least amount of code to do it in. Thus I make use of the sibling function in order to write less and do more. See the following:

Script

$(function() {
    $(".pageNumbers a").on("click", function(e) {
        //  Add highlight to the element clicked and remove highlighting from its siblings
        $(this).addClass("highlight").siblings().removeClass("highlight");
        //  Make use of our data attribute to show the correct page and hide the siblings
        $($(this).data('page')).show().siblings(".page").hide();
    });

    //  Finally, dynamically click first page to start things off for the user and provide proper highlighting and showing of text
    $("#a-1").click();
});​

See working jsFiddle

HERE

More instruction found on script section there.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks @spYk3HH. I guess it will makes things easier in the long run to learn jQuery. \$\endgroup\$
    – soupman55
    Commented Jul 20, 2012 at 5:46
3
\$\begingroup\$

Here are some ideas:

  • Use either tab or, preferably, 4-spaces for indention consistently. Right now the indention is all screwy on JSBin.
  • Put all your variable declarations at the top, even if you don't define them yet. This'll prevent any confusion from variable hoisting.
  • Since null is a falsy value, you can change document.getElementById(...) !== null to document.getElementById(...) to check for the existence of an element in you if statements.
  • Space out your if statements (so if (...) instead of if(...).)
  • Maybe think about using jQuery? It'll make working with events and elements much easier than using the DOM directly. So for example, hiding all the pages could be written as easy as: $('div').hide().

That's all I have time for now!

\$\endgroup\$
0

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.