SitePoint Sponsor

User Tag List

Results 1 to 3 of 3
  1. #1
    SitePoint Wizard Stomme poes's Avatar
    Join Date
    Aug 2007
    Location
    Netherlands
    Posts
    10,278
    Mentioned
    50 Post(s)
    Tagged
    2 Thread(s)

    A bunch of appendChilds... a neater/tidier way of writing this?

    Hallo,

    I was asked (told) to make a popup advertisement. Yuck. But, whatever, I know how to stop caring about my code in situations like this. Still, an opportunity to better my Javascript anyway.

    Because a back-end script first checks to see if the person is new visitor, the script tag gets added into the HTML by the back-end, so it made most sense (I think) to have the script build the popup entirely, instead of having HTML there for everyone. I don't care if sensible people have a popup blocker and I don't care if non-JS'd people/the googles don't see it. The current boss only cares about Windows with IE anyway.

    So the script builds the box, and a Close button on it just manually changes the display to "none", mostly because it didn't make sense to play with classes for such a one-time throwaway element. I do however have an awful lot of appendChilds and it doesn't look very nice. Tutorials tend to show it the way I've done it so maybe, I dunno...

    Here is the current, working code:
    Code:
    var EvilPopup = {
        init: function() {
            
            var container = document.getElementById('container'),
                div = document.createElement('div'),
                ad = Basis.createAnchor('Word abonnee van <some company>', 'ad', 'http://example.com'),
                close = Basis.createAnchor('sluiten', 'close', '#void'),
                bannerImg = Basis.createImg('Word nu abonnee van <some company> voor slechts 5 per jaar', '250', '250', 'images/Banner_abonnee.jpg'),
                closeImg = Basis.createImg('Sluiten', '16', '16', 'images/close.png');
                
                div.id = 'shlEvilPopup';
                ad.appendChild(bannerImg);
                close.appendChild(closeImg);
                Basis.addEventListener(close, 'click', EvilPopup.hide);
    
                div.appendChild(ad);
                div.appendChild(close);
    
                container.parentNode.insertBefore(div, container);
        },
    
        hide: function(event) {
            var div = this.parentNode;
            div.style.display = 'none';
            return;
        }
    };
    
    Basis.begin(EvilPopup);
    It creates this (bold) HTML:
    Code:
    <body>
      <div id="shlEvilPopup">
        <a href="http://example.com" id="ad" title="Word abonnee van <some company>"><img src="Banner_abonnee.jpg" alt="Word nu abonnee van <some company> voor slechts &amp;#8364;5 per jaar" height="250" width="250"></a>
        <a href="#void" id="close" title="sluiten"><img src="close.png" alt="Sluiten" height="16" width="16"></a>
      </div>
      <div id="container">
    ...
    (alt text for when there's no image, title text on the anchors for keyboarders to know the text normally only mousers get, with some CSS help)
    All the object stuff and init blah blah is partially because I know other scripts will appear later and at some point I'm going to have to incorporate them all into this... right now, it looks like a bit of overkill for what it's doing.

    the helper functions inside Basis:
    Code:
    ...
        Basis.createAnchor = function(title, id, href) {
            var anchor = document.createElement('a');
            if (title) {
                anchor.title = title;
            }
            if (id) {
                anchor.id = id;
            }
            anchor.href = href;    
            return anchor;
        }
        
        Basis.createImg = function(alt, h, w, src) {
            var img = document.createElement('img');
            img.alt = alt;
            img.height = h;
            img.width = w;
            img.src = src;
    
            return img;
        }
    
    ...
    My question is, is there a more elegant way to build an element, or is it just JS verbosity to have all these appendChilds? I thought of what I've learned of documentFragment but that's appending a bunch of stuff at the same level... here I'm putting things inside of things inside of things. Any other places this code could have been better written (removing the display = "none" aside)?

    I also have a side question: when I'm listing a bunch of arguments like above, and they're all being filled with strings, I understand that if I don't fill ALL the args that's ok, the last will be ignored, but there was also the idea of doing this:

    Basis.createAnchor = function(href, id, title, text)

    Well, if I'm later creating an anchor who has text but no title, the script would not know which was which in this case:

    var someAnchor = Basis.createAnchor('url', 'theID', 'some anchor text')

    "title" is the third arg in the creation function... so would it assume that's title text? If so, does this mean when I have any number of possible missing attributes, I cannot do my helper function this way (should I instead gamble on the setAttribute thing even though IE is evil with it?)?

    (Currently, all created anchors will have an href and an id, even though I have the "if (id)" part in there. In this particular case of the popup, there will always be a title and never any anchor text... so my question is a general one.)

  2. #2
    I meant that to happen silver trophybronze trophy Raffles's Avatar
    Join Date
    Sep 2005
    Location
    Tanzania
    Posts
    4,662
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    I'm afraid that is as minimalistic as it's going to get. In reality it's actually more verbose because of your Basis.createAnchor little abstraction. The only way to do it in fewer characters is with innerHTML.

    I have to point something out here though.

    Code javascript:
    // This:
     
        hide: function(event) {
            var div = this.parentNode;
            div.style.display = 'none';
            return;
        }
     
    // can become this:
     
        hide: function(event) {
            this.parentNode.style.display = 'none';
        }

    The return is especially redundant, as that is what is going to happen anyway at the end of the function.

    Well, if I'm later creating an anchor who has text but no title, the script would not know which was which in this case:
    Yes, so you have to do this:

    Code:
    var someAnchor = Basis.createAnchor('url', 'theID', null, 'some anchor text')
    And in your code you must check which arguments have been passed as useful values:

    Code:
    if (title) {
      // do stuff with title
    }
    So you can omit passing arguments, but only if they don't have any useful arguments following them. Otherwise you'll confuse the function. Of course, omitting arguments at the end can also be a problem, if the function is expecting them.

  3. #3
    SitePoint Wizard Stomme poes's Avatar
    Join Date
    Aug 2007
    Location
    Netherlands
    Posts
    10,278
    Mentioned
    50 Post(s)
    Tagged
    2 Thread(s)
    Thanks for the feedback. D'oh! on the hide function.

    In reality it's actually more verbose because of your Basis.createAnchor little abstraction. The only way to do it in fewer characters is with innerHTM
    Yeah, I was looking at that. Since I know later I'll have to incorporate a few more scripts into the site, and they'll be using the funclibrary, I kept them. Overmodularisation == 600-line Hello World lawlz. : )


Tags for this Thread

Bookmarks

Posting Permissions

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