Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a web page with a set of icons that are grey by default:

enter image description here

If a user clicks one, it toggles green (.toggleClass('green')):

enter image description here

If a user clicks a green one, it toggles back to grey (.removeClass('green')).

Only one icon can be green at any given time, so if you have a green icon and you click a grey icon, the icon that was originally green will change to grey while the one you clicked would change to green.


Each of these icons corresponds to a section on the page.

  • If all the icons are grey, all the sections on the page are visible.

  • If one icon is green, only that icon's section is visible, the sections represented by the grey icons are hidden.


Here is the code that controls that behavior:

$(document).on 'click', '.toggler', ->
  icons = [$('#toggle-videos'),$('#toggle-images'),$('#toggle-words')]
  sections = [$('#videos'),$('#images'),$('#words')]
  if $(this).hasClass('green')
    $(this).toggleClass('green')
    i = 0
    while i < sections.length
      sections[i].show()
      i++
  else
    $(this).addClass('green')
    i = 0
    while i < icons.length
      if icons[i][0] != $(this)[0]
        icons[i].removeClass('green')
        sections[i].hide()
      else      
        j = 0
        while j < sections.length
          if $(this).attr('id').indexOf(sections[j].attr('id')) != -1
            sections[j].show()
          j++
      i++

I feel it is very hard to understand what this code does simply by reading it. I feel I am missing a simpler way to accomplish the following:

  • Associate an icon with a specific section on the page
  • Keep the green state of an icon associated with the visibility of a specific section on the page

My primary concerns with the code:

  • I have several nested if statements with nested while statements
  • I am checking the relationship between a section and an icon by checking if the icon.id has part of the string (this is the icon element and j is a counter): if $(this).attr('id').indexOf(sections[j].attr('id')) != -1

Is this acceptable for javascript or is there a better way?

share|improve this question

5 Answers 5

up vote 2 down vote accepted

jQuery generally operates on collections of elements, so you rarely need to old-fashioned loops and incrementing indexes. In addition, CoffeeScript has many syntactic niceties for dealing with collections, which means that you hardly ever have to do old-fashioned looping.

I'd suggest keeping the link-to-section relationship in the HTML, like so:

<a class="toggle" href="#videos">...</a>
<a class="toggle" href="#words">...</a>
<a class="toggle" href="#images">...</a>

(using href has the added benefit that it'll still work, even with no JavaScript support)

Then, use that in your code to hide/show what's needed, e.g.

$ ->
  toggles  = $ ".toggle"
  sections = $ ".section" # or whatever selector works

  toggles.on "click", (event) ->
    event.preventDefault()

    target  = $ this
    section = $ target.attr("href") # get the associated section

    if target.hasClass("selected")
      target.removeClass("selected")
      sections.show() # show all sections
    else
      toggles.removeClass("selected") # remove selection from other links
      target.addClass("selected")     # select the clicked link
      sections.not(section).hide()    # hide other sections
      section.show()                  # show only the relevant section

Here's a demo

I've changed the selection class to selected rather than green, because "green" isn't a very descriptive name - or, in a sense, it's much too descriptive. Either way, the name's tied to the look rather than the purpose/function of the class.

share|improve this answer
    
Good point about the 'green' to 'selected' - I should have made that change before I even posted this here. Someone else mentioned using data attributes, it seems you use the actual href attribute to fulfill a similar role. I like this because it would give feedback on mouse-hover about what is going to happen. –  Ecnalyr Apr 4 at 13:19
1  
@Ecnalyr Using data attributes is a perfectly valid solution too. I only use the href attribute here, because what we need to store - an element selector - has exactly the same syntax as a regular anchor link. Otherwise, I'd use a data attribute too. But, as mentioned, the links will make the page scroll to the correct section even if you remove all the javascript/coffeescript. I.e. it degrades gracefully –  Flambino Apr 4 at 13:24
    
Also nice trick with sections.not(section). Probably doesn't make much sense in this case. But is more correct if show/hide will be replaced with fadeIn/Out e.g. –  Stepan Stepanov Apr 4 at 13:30
    
@StepanStepanov That depends how you the look & feel you want for the page; the original code just uses show/hide. But yes, you can just replace it with fadeIn/Out, if you want. –  Flambino Apr 4 at 13:38

I would propose to use data attributes. E.g. icons have data-section-id attribute.

if $(this).hasClass('green') // all should become greynow
{
    $('div.section').show(); // show all sections
    $(this).removeClass('green');
}
else // we turn one sectio on
{
    $('div.section').hide(); // show all sections
    $('#'+ $(this).data('sectionId')).show(); // show a corresponding section
    $('li.icon').removeClass('green'); // turn all icons off
    $(this).addClass('green'); // turn current one on
}

But of course, that requires some changes in HTML structure.

share|improve this answer
    
I have been using data attributes elsewhere in the same app to come up with a similar solution - glad to see it came up here. Not a bad idea. –  Ecnalyr Apr 4 at 13:14

If you like fun jQuery chains, the following should work for you.

$(document).on 'click', '.toggler', ->
    targetSection = $(this)
        .parent()
        .find('green')
            .removeClass('green')
            .end()
        .end()
        .addClass('green')
        .data( 'target' )
    $('.section').not(targetSection).hide()
    $(targetSection).show()

One thing is that you would have to modify your icons to have data- attributes like so:

<a id="video-toggle" data-target='#videos'>...</a>
share|improve this answer

I usually don't review CoffeeScript, because I find it too hard too read. Your code is not too hard to read, so I will give it a shot.

I think in essence, you are trying too hard to show all but the one section, I would simply had all sections, and then show the one section you need. I would also cache $(this) for speed reasons and use more [].forEach for readability. Finally, this.id is more readable and faster than $(this).attr('id')

This untested counter proposal should illustrate what I mean:

$(document).on 'click', '.toggler', ->
  //Look up by `id`
  icons = ['toggle-videos','toggle-images','toggle-words']
  //Show via '#` + id
  sections = ['#videos','#images','#words']
  $this = $(this);
  if $this.hasClass('green')
    $this.removeClass('green')
    sections.forEach( (section) => section.show() )
  else
    $this.addClass('green')
    sections.forEach( (section) => section.hide() )
    sections[ icons.indexOf( this.id ) ].show(); 

Personally, and that is a bit more hackish, I would abuse the fact that the section id is postfixed to the toggle id and do this:

$(document).on 'click', '.toggler', ->
  $this = $(this);
  if $this.hasClass('green')
    $this.removeClass('green')
    sections.forEach( (section) => section.show() )
  else
    $this.addClass('green')
    sections.forEach( (section) => section.hide() )
    sections[ this.id.split("-")[1] ].show();

I might be tempted to put a toggleClass in there as well, but somehow the if and then the removeClass or addClass make this code easier to parse for me.

share|improve this answer
    
I did not know I could cache $(this) (I only 'do javascript' when I HAVE to). You made some good points about performance, hadn't even considered it up to this point. –  Ecnalyr Apr 4 at 13:16
    
It's interesting you find CoffeeScript harder to read; I have the exact opposite experience. These days I only write plain JS for CodeReview answers :) –  Flambino Apr 4 at 14:51
    
I think I'm getting old and (c)rusty ;] –  konijn Apr 4 at 15:19

To start with, you shouldn't use "green" as a class name. The name should be something generic, like "selected" or "current". Your CSS will cause the current selection to be rendered as a green icon, but you shouldn't hard-code that assumption into the name of the CSS class itself.

What you have described is essentially a radio button (with the additional feature that clicking on the currently selected item will deselect everything). Therefore, if you start with radio buttons, styled with images using CSS, most of your work will be done already!

share|improve this answer
    
I would like the radio button idea to work - and it would ordinarily. But I've failed to style with iconfonts in the past (probably my fault, not the fault of css itself), so I always stray away from this idea. I may have to revisit it. –  Ecnalyr Apr 4 at 13:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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