Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Page 1 of 2 12 LastLast
Results 1 to 15 of 16
  1. #1
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts

    Check-boxes Contradicting Each Other

    Hello,
    I'm working on a project that uses check-boxes to narrow down a selection.

    Just want to say this 1st, this is my first attempt at JavaScript and I am not comfortable using jQuery. I'm trying to write as much of this myself as possible.

    Ok that out of the way here goes. Here is a link to what I've been able to come up with so far:

    http://jsfiddle.net/g_borg/48uhn/1/

    This is a list of Attributes(check-boxes) for specific grey boxes. e.g. check the 'Economy' check-box, only the 'Bernini' grey box is visible and any check-boxes that do not apply to the 'Bernini' grey box become grayed out and disabled. That works.

    But there are some check-boxes that apply to more than 1 grey box. For instance the 'Tape-in Bottom Rail' check-box applies to both the 'Bernini' and 'Picasso' boxes, thus if that is checked, both boxes are visible. The issue I'm having is if a user checks the 'Economy' check-box, then the 'Tape-in Bottom Rail' check box, but then decided to uncheck the 'Tape-in Bottom Rail' box, check-boxes that should be disabled and grayed out are not. I think this is a matter of a double negative making a positive but don't know how to fix this. I tried using 'else if' statments instead of else if else if over and over but that didn't seem to help.

    I know that I need to get the function to check what is already checked before changing the display of the check-boxes but I am unaware of how to do so.

    Any help would be greatly appreciated. Thank you!

  • #2
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    You are doing several things right: learning Javascript from scratch before looking at jQuery and presenting a self-contained, working example in form of a fiddle. Bravo, Borgard, your question has a much better quality than most other beginner's questions.

    That being said, you sure picked an interesting and non-trivial example to start out. And maybe it's really not the best example for a beginner – but what the heck, you're in it now and you already got this far.

    I think the reason no one answered so far is because it takes some effort to dig into your code and try to figure out what's wrong. I didn't look at it in all detail either, but the issue is pretty clear: whenever a checkbox is un-/checked, you just execute the actions attached to that checkbox – and silently ignore the state of all other checkboxes that might still be checked.

    It would solve your problem and reduce the complexity if you were to just go through all(!) checkboxes and decide what to do whenever any checkbox is un-/checked. In other words, when a checkbox is un-/checked, reset everything and start disabling stuff from scratch again. I hope you get what I mean.

    I refactored your code a lot, since all functions are basically doing the same, just for different elements. By all means, this code is nowhere near "pretty", but surely a lot easier already. It also most likely doesn't work the way you want it to quite yet, but I hope it can be a good base for you to continue working with. I had to stop working on it because I gotta go now Oh and excuse my silly names for the variables. Not understanding the meaning of your project just makes it harder to know what terms to use.
    I also added the "reset" already, however: the logic of "going through every checkbox" is not(!) added. I also didn't add changing any colors as you did yet. I had to stop because I need to go now, but I hope it already helps. Of course feel free to ask any questions.

    http://jsfiddle.net/48uhn/3/

    If it's too hard to understand, I can continue on this tomorrow.
    Last edited by Airblader; 11-13-2013 at 09:51 PM.

  • Users who have thanked Airblader for this post:

    Borgard (11-13-2013)

  • #3
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    Thank you! Now it may take me some time to look over what you've written, and I hope my head doesn't explode but I'll see what I can understand. I should have though to make an array of the variables as you did, would have saved me some coding time.

    I have been working on this thing all day today and I think I have it working (happened before I read your post). I've been trying to make it fail and will have to get another person to actually test this out as my mind is numb from looking at code for the past few days.

    http://jsfiddle.net/g_borg/TWzRF/1/

    Here is the updated version. I tried to address the lack of checking other check-boxes, but the only way I was able to get that to look like its working is to do if, else/if else/if else statements if that makes sense, nesting if statements? Is that a correct term?

  • #4
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    You are quickly experiencing what is known as "code smell". Your code contains a lot of duplication and is hard to follow and maintain. That makes any small change dangerous, as it can easily break some particular case – and you noticed yourself how hard it is to keep track and "try out" every case just to see it's working right. And also every change now means adding onto the pile of code, bloating it up more and more, which basically is a vicious circle.

    This is exactly why you should simplify your code in a similar manner as I did (you can take it much further) and extract more functions to reuse. Through this abstraction you only need to understand very little code to cover every(!) case.

    I'll be happy to continue working on this tomorrow and would really advice against working on and adding on to your code. It's great you got this far and you learned a lot, but it's the right moment to turn your attention to simplifying if you want to be sure everything is really working the way you want it to. Like I said, you are already experiencing the problems with all this duplication yourself.

    (And yes, "nesting if statements" is a perfectly fine term)
    Last edited by Airblader; 11-13-2013 at 09:58 PM.

  • #5
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    I would really appreciate it if you had some time to look at that code. I'm going to have to take a break (the night) off from looking at this. But try to understand how you started simplifying it as it looked much cleaner.

    Thank you again!

  • #6
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    No problem. I have to go to sleep too. We shall continue this tomorrow

  • #7
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    I worked on it "a bit" and tried to simplify it as much as I can. Before going into any explanations, I think it'd be best if you

    • verified it actually does what it's supposed to
    • tried to understand as much of it as you can


    Then I will happily answer all your remaining questions. Just a few things I want to point out:

    One big thing I did was Separation of Concerns (SoC): in your code you had both inline CSS and inline Javascript in your HTML. You should avoid that. As a general rule: Any file should contain at most two languages (one of which 99% of the time will be English, so there is only room for one programming language).
    I also removed a lot of logic that I (hopefully rightfully) determined to be obsolete and also removed the logic of coloring disabled checkboxes (this is done with pure CSS now).

    http://jsfiddle.net/k6c6E/

  • Users who have thanked Airblader for this post:

    Borgard (11-14-2013)

  • #8
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    Wow,
    Sorry took a bit to get back here today, been busy. I'm looking at this locally now (copy & pasted the code from fiddle). Maybe I just need my coffee today but;

    Am I correct in seeing there is not direct function attached to the check-boxes? Instead it is attached to the element id 'wrapper' - selector 'input...checkbox'?

    Then you made a config variable that consisted of each of the different functions I had going on, so 'Bernini' is the specific checkboxes 3, 10, etc and the 2 specific grey boxes that won't display? Then the same thing for each of what I had as functions yesterday?

    And all the "magic" really happens in the handleCheckbox funtion?

    I'm still trying to digest this, it makes total sense but there is quite a bit here I have never seen nor would have thought of.

    I tried copying and pasting your CSS, HTML and JS code to new local files but when linked I cant' seem to get it to work.

    And THANK YOU this has been a huge help!

  • #9
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    Hi,

    don't sweat it, it's no race. First off, to make it work you need to make sure that the Javascript is executed after all the HTML has loaded. This is done most easily by putting it not in the head, but right before the body ends:

    Code:
    <html>
    <head>
        <link rel="..." />
    </head>
    <body>
        ... Here goes all the HTML ...
    
        <script type="text/javascript" src=".."></script>
    </body>
    </html>
    This is actually where your Javascript should go most of the time!

    Quote Originally Posted by Borgard View Post
    Am I correct in seeing there is not direct function attached to the check-boxes? Instead it is attached to the element id 'wrapper' - selector 'input...checkbox'?
    No, not quite. The line

    Code:
    var allCheckboxes = document.getElementById( 'wrapper' ).querySelectorAll( 'input[type="checkbox"]' );
    will just find all checkboxes within the element with id "wrapper" and store that list in the array allCheckboxes. Then at the bottom of the code I go through this list and attach an onChange listener to every checkbox. So each checkbox still does have a handler, but they all use the same function rather than different ones.

    Then you made a config variable that consisted of each of the different functions I had going on, so 'Bernini' is the specific checkboxes 3, 10, etc and the 2 specific grey boxes that won't display? Then the same thing for each of what I had as functions yesterday?
    Yes! Using this little config variable allows me to get rid of all these functions you had, because they all did the same thing, only that they used different ids. So by extracting it into a variable I could "melt" them all down into one single method.
    It's also nice because it makes it much easier to modify the code – say you want to have another checkbox activate "Picasso", then you only have to update the config rather than working your way through the entire code to find the one spot where you had to change it before. It makes the code easier to understand and more maintainable.

    And all the "magic" really happens in the handleCheckbox funtion?
    Indeed! Basically, what happens now when the user clicks any checkbox is:
    • Everything is reset (all checkboxes enabled, all grey boxes are shown)
    • Now we loop through every checkbox that is checked and execute handleCheckbox for it
    • handleCheckbox disables the checkboxes/grey boxes it is configured to disable


    So basically we rebuild the result from scratch whenever the user does something. That way we don't have to make complicated checks to see what we have to en-/disable.

    I'm still trying to digest this, it makes total sense but there is quite a bit here I have never seen nor would have thought of.
    That's okay. I'm actually surprised how far you got with this on your own, seeing how you are a beginner. I can tell you put quite some sweat and thought into your code – which is good. But it ended up piling up code, making it more messy as you went along. But don't worry, that's perfectly normal for beginners. I figured taking some complexity out of it is the right thing to do now.


    Please do ask any questions you have. It's what we're here for!
    Last edited by Airblader; 11-14-2013 at 07:29 PM.

  • #10
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    This has been a task indeed. I should have thought about the link to the script being at the end of the html, *bang head on desk*. I am having a little trouble trying to adjust 2 things, which I think I need to just step away for a bit, or sleep on it and go at it again in the morning.

    I am trying to set a "reset" button. I had one working with the code I was originally working with. I haven't been able to get more than just re-enabling the check-boxes, not resetting them but making where they are no longer grayed out. My reset so far cannot; actually reset the check-boxes nor reset the grey-boxes and make them all visible again.

    Also, I'm not really understanding how the grey-boxes don't shift to the left when not visible. I would initially think with the float on them that they would just shift over to the left if one became hidden. Or is it still there just not visible as opposed to being display="" making it gone?

    Again thank you for your time on this.

  • #11
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    For a reset button you pretty much just need to call resetAll() and uncheck all checkboxes. So just place your button with, say, id "btnReset", wherever you want and then underneath the loop that attaches the onchange handler to the checkboxes, just add

    Code:
    document.getElementById( 'btnReset' ).onclick = function () {
        resetAll();
        for( var i = 0; i < allCheckboxes.length; i++ ) {
            allCheckboxes[i].checked = false;
        }
    };
    The reason the grey boxes don't shift anymore is indeed because I am using visibility instead of display now. Obviously if you łant them to shift, just go back to using display – I just thought it looks nicer and less confusing this way. Your explanation is correct: display actually makes the element disappear with all the space it would usually fill, visibility just makes it invsible, but acts as if the element was still there in terms of space.


    By the way: I should note that the code is not working cross-browser. For example because of querySelectorAll(). This would be the reason to use jQuery, because it will take cross-browser concerns off your hand. But like I said, first learning vanilla Javascript is absolutely a good thing.

  • Users who have thanked Airblader for this post:

    Borgard (11-15-2013)

  • #12
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    Quote Originally Posted by Airblader View Post
    For a reset button you pretty much just need to call resetAll() and uncheck all checkboxes. So just place your button with, say, id "btnReset", wherever you want and then underneath the loop that attaches the onchange handler to the checkboxes,

    That's what I was doing wrong. Ugh. I thought I had the reset right, but I made it a function separate from the rest at the top of the JS.

    Ok, I'll apologize for my ignorance on this but when you say it doesn't work cross-browser, what exactly do you mean? I can view and test this in lots of different browsers with no issues that I've noticed. So I must be using that term incorrectly.

  • #13
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    A quick look on caniuse – querySelectorAll will reveal that this method won't work in IE 7 and below – so any of those browsers will fail. It also says it for Firefox 2 and 3, but I doubt anyonse is still using those.

    Of course this is only querySelectorAll. If you want to be certain about cross-browser, you would have to check every method you are using.

  • #14
    New Coder
    Join Date
    Aug 2011
    Posts
    14
    Thanks
    5
    Thanked 0 Times in 0 Posts
    Whew! Ok I was getting worried about being able to use this live. Should I be concerned with older versions of IE? Older as in older that 8? Looking at w3schools browser statistics the % is really low.

    Again Airblader I can't thank you enough for all your help!

    I'm going to try to edit this some more with some real content inside the greyboxes so it will make more sense. Can I ask more questions of you as they may come up?

  • #15
    Regular Coder
    Join Date
    Jan 2013
    Location
    Germany
    Posts
    578
    Thanks
    4
    Thanked 77 Times in 77 Posts
    Whether or not you should worry really depends on who you ask. I, personally, wouldn't care for IE7 and earlier. But profesionally, I probably would have to care (though I profesionally don't work on websites, so it never comes up).

    And yes, of course you can keep asking questions. I encourage you to!

    P.S.: You shouldn't use w3schools. That site is so out of date in many places that it became plain wrong. There even is a site dedicated to educating people to not using it: www.w3fools.com


  •  
    Page 1 of 2 12 LastLast

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •