Interupt redirect to catch potential newsletter signup

Question

This is a piece of code I am working on to reuse in a few places on various websites. It is used to capture name and email addresses for people interesting in registering for something, we want to add them a newsletter...So here is the scripts/CSS/HTML and it feels clunky. And maybe others might have some input on how I can do this better.

The Javascript (jQuery):

$(document).ready(function () {
$("img").each(function (i) {
    var imgUrla = $(this).closest("a").attr('href');
    $("a img").attr('data-link', imgUrla).attr('id', 'target');
    $(this).closest("a").contents().unwrap();
});
$('img#target').click(function (e) {
    var imgUrl = $(this).attr('data-link');
    $('#noThanks').attr('onclick', 'opentab("' + imgUrl + '");');
    $('#thanks').attr('value', imgUrl);
    var posTop = 200;
    $('body').append('<div id="overlay"></div>');
    $('#achi').wrap('<div id="lightbox">').removeClass('newHidden');
    $('#lightbox').css('top', posTop + 'px');
    $('#overlay').fadeTo(500, 0.75, function () {
        $('#lightbox').fadeTo(250, 1);
    });
});
$('#overlay, #lightbox_close').live('click', function (e) {
    $('#achi').unwrap('<div id="lightbox">').addClass('newHidden');
    $('#overlay').remove();
    $('#lightbox_close').remove();
});
$("#submit").click(function () {
    $('form').submit(function () {
        var faults = $('input').filter(function () {
            return $(this).data('required') && $(this).val() === "";
        }).css("background-color", "pink");
        if (faults.length) return false;
    });
});
});

The code above does a few things. It looks for any image with hyperlinks and strips the tag then moves the href portions (the URL) into the image as a parameter called data-link. The next thing it does is when the image is clicked, it presents a floating div/form to capture the name and email of the user with cancel and submit. If they hit submit and the fields are not filled then it errors (it is a simple validation), if the user hits cancel then it shoots them to the redirect url in the data-link img parameter.

Here is the CSS:

#overlay {
    position: fixed;
    top: 0;
    left: 0;
    background-color: #000000;
    width: 100%;
    height: 100%;
    z-index: 998;
    display: none;
}
#lightbox {
    position: fixed;
    z-index: 999;
    width: 590px;
    text-align: center;
    display: none;
    background-color: #fff;
    left: 50%;
    margin-left: -285px;
}
.newHidden {
    display: none;
}
img {
    width:100px;
    height:auto;
}

The CSS above was very specific and somewhat difficult. I had to get help with it because the overlay and form were not behaving right. Everything in there may be necessary with the exception of the img width and height (which I added to scale the images.

The HTML:

<!--hidden div/form-->
<div id="achi" class="newHidden">
     <h2>Be notified!</h2>
    <form id="" action="" name="hiddenForm" method="post">
        <label>Name</label>
        <br />
        <input type="text" value="" name="name" data-required="true">
        <br />
        <label>Email Address</label>
        <br>
        <input type="text" value="" name="emailAddress" data-required="true"> <b class="clear">
        <div style="width:485px;margin:0 auto;margin-bottom:60px;">
            <button type="button" class="left noThanksButton" id="noThanks" name="noThanks">No Thanks</button>
            <button type="submit" class="right" id="submit" name="submit">Submit</button>
        </div>
        </b>
        <input id="thanks" type="hidden" value="" name="URLAddress">
    </form>
</div>
<!--hidden div/form-->
<h3>Links</h3>
<p>Come explore!</p>
<p><a href="http://google.com/">
<img class="colorboxd" src="https://www.google.com/images/srpr/logo11w.png"/>
</a>
</p>
<h3>August Wild Night!</h3>
<p>Come discover the animals and birds!.</p>
<p><a href="http://google.com/">
<img class="colorboxd" src="https://www.google.com/images/srpr/logo11w.png"/>
</a>
</p>
<h3>September 19 We're Grounded Again!</h3>
<p>This month we'll explore creatures that love to creep and crawl...</p>
<p><a href="http://google.com/">
<img class="colorboxd" src="https://www.google.com/images/srpr/logo11w.png"/>
</a>
</p>

The HTML above has the div with the form to capture user data. It is set to hidden from the get go. The link stuff above is there to model against and is just for testing.

So what I am looking for is code enhancement suggestions. Maybe shorthanding some of that jQuery. It would really be nice to push the CSS stuff into the jQuery (or have it write it so I can eleminate the CSS, I want to eleminate the HTML to and have the script build it.

But first I want to refine the jQuery and anything else that could be improved. So please, make suggestions. And if you have any questions please let me know.

Here is the jsfiddle

Here is my revised code using suggestings by the two guys that helped me out below:

The Javascript:

$(document).ready(function () {

function minMod_subscriber(){
    var minMod_subscriber = '';
    minMod_subscriber +='<div id="minMod_achi" class="minMod_newHidden">';
    minMod_subscriber +='<h2>Be notified!</h2>';
    minMod_subscriber +='<form id="" action="" name="hiddenForm" method="post">';
    minMod_subscriber +='<label>Name</label>';
    minMod_subscriber +='<br />';
    minMod_subscriber +='<input type="text" value="" name="name" data-required="true">';
    minMod_subscriber +='<br />';
    minMod_subscriber +='<label>Email Address</label>';
    minMod_subscriber +='<br>';
    minMod_subscriber +='<input type="text" value="" name="emailAddress" data-required="true">';
    minMod_subscriber +='<b class="minMod_clear">';
    minMod_subscriber +='<div style="width:485px;margin:0 auto;margin-bottom:60px;">';
    minMod_subscriber +='<button type="button" id="minMod_noThanks" name="noThanks">No Thanks</button>';
    minMod_subscriber +='<button type="submit" id="minMod_thanks" name="submit">Submit</button>';
    minMod_subscriber +='</div>';
    minMod_subscriber +='</b>';
    minMod_subscriber +='</form>';
    minMod_subscriber +='</div>';
    return minMod_subscriber;
}

$('body').append(minMod_subscriber());

function minMod_stripper(){
    $("p a img").each(function (i) {
        var imgUrla = $(this).closest("a").attr('href');
        $("p a img").attr('data-link', imgUrla).attr('id', 'minMod_target');
        $(this).closest("a").contents().unwrap();
    });
}

minMod_stripper();

function unwrapOverlays() {
    var imgUrl = $(this).attr('data-link');
    window.open(imgUrl, '_blank');
    $('#minMod_achi').unwrap('<div id="minMod_lightbox">').addClass('minMod_newHidden');
    $('#minMod_overlay').remove();
    $('#minMod_lightbox_close').remove();
}

function checkinputs() {
    var checkinputs = true;
    $("input").each(function () {
        var val = $(this).val();
        if ($(this).attr('data-required') && val === "") {
            $(this).css("background-color", "pink");
            event.preventDefault(); // Stop the submit here!
            checkinputs = false;
        }
    });
    return checkinputs;
}

$('#minMod_noThanks').on('click', function () {
    unwrapOverlays();
});

$('#minMod_thanks').on('click', function () {
    if (checkinputs()) {
        unwrapOverlays();
    }
});

$('img#minMod_target').click(function (e) {
    var posTop = 200;
    $('body').append('<div id="minMod_overlay"></div>');
    $('#minMod_achi').wrap('<div id="minMod_lightbox">').removeClass('minMod_newHidden');
    $('#minMod_lightbox').css('top', posTop + 'px');
    $('#minMod_overlay').fadeTo(500, 0.75, function () {
        $('#minMod_lightbox').fadeTo(250, 1);
    });
});
});

The CSS:

#minMod_overlay {
    position: fixed;
    top: 0;
    left: 0;
    background-color: #000000;
    width: 100%;
    height: 100%;
    z-index: 998;
    display: none;
}
#minMod_lightbox {
    position: fixed;
    z-index: 999;
    width: 590px;
    text-align: center;
    display: none;
    background-color: #fff;
    left: 50%;
    margin-left: -285px;
}
.minMod_newHidden {
    display: none;
}
.minMod_clear {
    clear:both;
}

The HTML:

<!--test code-->
<h3>Links</h3>
<p>Come explore!</p>
<p> 
<a href="http://google.com/">
<img src="https://www.google.com/images/srpr/logo11w.png"/>
</a>
</p>
<!--test code-->

A few notes: I made more unique CSS IDs and Classes. I also created more functionality in the JS. I moved the form build into JS. I also improved the a href targets. So links have to be in p tags, then a tags, and have img tags in that order (to prevent a bunch of unforeseen targeting).

Here is the jsfiddle!

Last thoughts. In my next iteration I will make things more customizable for example. Coloring of the background to the modal, and several other things, like text overrides, etc.

Answer 1

This is a piece of code I am working on to reuse in a few places on various websites

This line alone tells me that it's a drop-in script. And that alone makes me tell you to skip jQuery altogether. Never assume a website uses jQuery. Also, not all systems can run jQuery. It could be due to compatibility with another library, an older version of jQuery or license restrictions. Who knows?

If you want to keep this script versatile (and sneaky):

• Skip jQuery, create your own abstractions. This keeps your script flexible yet light.
• Ship a custom build of jQuery, with only the things you need. Not the entire library.
• Drop-in scripts should be as simple as adding a <script> for the user. Use a "trojan-style" resource loading script instead. Basically it's a single script that loads all other stuff required. This is how Google scripts (API, Analytics) work. Resources include JS, CSS, and even HTML.

The CSS above was very specific and somewhat difficult. I had to get help with it because the overlay and form were not behaving right.

It's what you call defensive programming. What prevents the user from clobbering your styles if you have selectors such as #overlay, img, #lightbox. These names are too simple, and the user's stylesheets might contain similarly named selectors. Their styles will clobber your styles, even override them.

Keep your selectors specific, and namespaced. for instance:

<div id="MyModalDialog">
  <span>text</span>
</div>

If you styled span like span{color:red} and the user's style has span{color:green} after yours, then your styles are overridden. What you do is add more selectors to keep it specific, like #MyModalDialog span{color:red}.

Also, by namespacing your style to #MyModalDialog, it keeps your styles focused on what's inside the #MyModalDialog and avoid clobbering the user's styles as well.

---

As for other stuff:

You don't declare handlers like this

$('#noThanks').attr('onclick', 'opentab("' + imgUrl + '");');

Do this instead:

$('#noThanks').on('click',function(){
  opentab(imgUrl);
});

Avoid applying CSS using JS

.css("background-color", "pink");

Instead, add a class that contains that style. Saves you debugging time looking for where pink is from.

.addClass('bg-pink');

bg-pink{
  background-color:pink;
}

Answer 2

Extending on what Joseph wrote abotu namespacing: Your script currently breaks basically any webpage you add it to.

 $("img").each(function (i) {

Why are you apply this to all images on the page? There are probably plenty of linked images, that shouldn't be using your script.

 $("a img").attr('data-link', imgUrla).attr('id', 'target');

With $("a img") applying this again to all (linked) images on the page. However you probably just want to work with the image of the current .each() loop. Why don't you just use $(this) again?

Also this set the same id to all images. Ids must be unique per page. And why do you need this ID in the first place? Later you only use it to assign a click handler. Why don't you assign the click handler right here?

$(this).closest("a").contents().unwrap();

You repeat $(this).closest("a") here. Generally (not only here): If you need a reference to an element multiple times, then store that reference in a variable, instead of searching for it multiple times.

Also you remove the link (again: all links around all images). What if the author needs this link, for example, for styling or he has his own event handler on it?

$('#overlay, #lightbox_close').live('click', function (e) {

What jQuery version are you using here? Are you seriously using an old version to develop a "new" script? .live() has been removed since jQuery 1.9.