2
\$\begingroup\$

I created a JS widget library for distribution purpose on external websites and would like your thoughts on it.

What widget does: It adds an always visible text on the screen. When user clicks on text, an external page gets loaded in iFrame in a lightbox. Internally, widgets loads jQuery library and some external JS and CSS files.

What I need to be reviewed:

  1. Check if prototype being used the right way and work across all browsers?
  2. If jQuery is correctly loaded and resolved correctly (even if the page hosting the script have same/different version of jQuery already running on the page)
  3. I used 'window.addEventListener' method to check if jQuery is loaded or not. Is it correct and the way ideally it should be?
  4. Is there any efficient way of loading the colorbox.js file
  5. Can the code be optimised further for improved performance?
  6. Is the code production ready?

HTML Code

<html>
<head>
    <link rel="stylesheet" type="text/css" href="style.css">
</head>
<body>
    <a href="http://yahoo.com">Anchor - by user code</a>
    <div style="height:1500px;"></div>
    <a href="http://yahoo.com">Another Anchor - by user code</a>

    <!-- Widget Script: Starts -->
    <script type="text/javascript" src="widget.js" ></script>
    <Script type="text/javascript"> 
        var customWidget = new MyWidget();
        customWidget.Render({ anchorText:'wikipedia', link: 'http://www.wikipedia.com/' });
    </script>
    <!-- Widget Script: Ends -->
</body>
</html>

JS Code - widget.js

var MyWidget = function () {

  // Loading CSS in Async
  var fileref = document.createElement("link");
  fileref.setAttribute("rel", "stylesheet");
  fileref.setAttribute("type", "text/css");
  fileref.setAttribute("href", 'widget.css');
  document.getElementsByTagName("head")[0].appendChild(fileref);

  fileref = document.createElement("link");
  fileref.setAttribute("rel", "stylesheet");
  fileref.setAttribute("type", "text/css");
  fileref.setAttribute("href", 'http://www.jacklmoore.com/colorbox/example4/colorbox.css');
  document.getElementsByTagName("head")[0].appendChild(fileref);

  // Localize jQuery variable
  var jQuery;
  // Load jQuery if not present
  if (window.jQuery === undefined || window.jQuery.fn.jquery !== '1.10.2') {
    var script_tag = document.createElement('script');
    script_tag.setAttribute("type", "text/javascript");
    script_tag.setAttribute("src", "http://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js");
    if (script_tag.readyState) {
      script_tag.onreadystatechange = function () { // For old versions of IE
        if (this.readyState == 'complete' || this.readyState == 'loaded') {
          this.ResolveJqueryAndLoadAdditionalJsAndCss();
        }
      };
    } else {
      script_tag.onload = this.ResolveJqueryAndLoadAdditionalJsAndCss;
    }
    // Try to find the head, otherwise default to the documentElement
    (document.getElementsByTagName("head")[0] || document.documentElement).appendChild(script_tag);
  } else {
    // The jQuery version on the window is the one we want to use
    jQuery = window.jQuery;
    this.ResolveJqueryAndLoadAdditionalJsAndCss();
  }
};

MyWidget.prototype.ResolveJqueryAndLoadAdditionalJsAndCss = function () {
  // Restore $ and window.jQuery to their previous values and store the
  // new jQuery in our local jQuery variable
  jQuery = window.jQuery.noConflict(true);
};

MyWidget.prototype.Render = function (data) {
  window.addEventListener("load", function () {
    jQuery(document).ready(function ($) {

      $.when(
        $.getScript("http://www.jacklmoore.com/colorbox/jquery.colorbox.js"),
        $.Deferred(function (deferred) {
          $(deferred.resolve);
        })
      ).done(function () {
        console.log('colorbox loaded');
        $('<a href="' + data.link + '" class="cleanslate custom widgetExternalPage" style="position: fixed;right:0px; top:50%; z-index:1000000000;-webkit-transform:rotate(270deg);-moz-transform:rotate(270deg);-o-transform:rotate(270deg);-ms-transform:rotate(270deg);transform:rotate(270deg);filter:progid:DXImageTransform.Microsoft.BasicImage(rotation=3);">' + data.anchorText + '</a>').appendTo('body');
        $(".widgetExternalPage").colorbox({
          iframe: true,
          width: "80%",
          height: "80%"
        });
      });
    });
  }, true);
};
\$\endgroup\$
2
  • \$\begingroup\$ some of your questions imply that you have not tested the code and that you don't know if the code works or not. \$\endgroup\$
    – Malachi
    Commented Jan 8, 2014 at 18:51
  • \$\begingroup\$ I have verified and its working for modern browsers, however, I still need to check for old versions of IE. As I'm not an expert on JS and couldn't find an example suitable suitable to my needs, that's why I posted. \$\endgroup\$
    – Ankur
    Commented Jan 9, 2014 at 5:57

2 Answers 2

Reset to default
4
\$\begingroup\$

Some observations:

  • ResolveJqueryAndLoadAdditionalJsAndCss is too long a function name
  • ResolveJqueryAndLoadAdditionalJsAndCss lies about what it does
  • jQuery version '1.10.2' seems awfully restrictive, are you sure ?
  • You copy pasted the code to load a css file, use a function
  • console.log in production code is bad
  • functions in the prototype and variables should follow lowerCamelCasing.

For your questions

  1. Check if prototype being used the right way and work across all browsers?

  2. If jQuery is correctly loaded and resolved correctly (even if the page hosting the script have same/different version of jQuery already running on the page)

    The only way to be sure is to test on all browser within pages using different versions of jQuery.

  3. I used 'window.addEventListener' method to check if jQuery is loaded or not. Is it correct and the way ideally it should be?

    That seems brittle to me, what if something else loaded first ? Also, do you re-render every single time something gets loaded ?

  1. Is there any efficient way of loading the colorbox.js file

  2. Can the code be optimised further for improved performance?

    Looks fine to me, not sure

6 Is the code production ready?

I think not yet, but close.

Update

Thoughts on making the jQuery requirements less brittle. Too start I would check whether jQuery was loaded or not before executing the rest. Maybe have a variable called renderingRequested and renderData and extract the actual rendering into a function called render and call your current render -> requestRender.

then requestRender would be something like : 

MyWidget.prototype.requestRender = function (data) 
{
  if( jQuery ){
    render( data );
  }
  else {
    renderingRequested = true;
    renderData = data;
  }
}

then, a renamed ResolveJqueryAndLoadAdditionalJsAndCss would do this

MyWidget.prototype.jqLoaded = function () {
  // Restore $ and window.jQuery to their previous values and store the
  // new jQuery in our local jQuery variable
  jQuery = window.jQuery.noConflict(true);
  if( renderingRequested ){
    render( renderData );
  }
};
\$\endgroup\$
1
  • \$\begingroup\$ / Malachi - Thanks. I'll make the changes as per your observation. Regarding 'window.addEventListener' can you recommend any other possible/ideal way? I can't use jQuery's document.ready() [only alternative I'm aware of] as I am using 'window.addEventListener' only to ensure that jQuery is already loaded. Any thoughts... \$\endgroup\$
    – Ankur
    Commented Jan 8, 2014 at 18:09
2
\$\begingroup\$

Focus on your widget

Do not load its dependencies. Instead, have the user of the widget to load them for you.

Real world scenario:

  • Bootstrap requires jQuery for it to work. Do you see Bootstrap load jQuery for itself? No, you don't.
  • Do you see jQuery plugins load jQuery for themselves? No, you don't.

In simple scenarios, you should keep things simple. Don't overcomplicate.

Flexibility and minimalism

Give the user a bit of flexibility as to where they want to place the link. Also, do it with as least code to the user as possible. You can do it jQuery style like so:

$('body').alwaysVisibleLink({
  text : 'Wikipedia',
  href : 'http://wikipedia.org/'
});

With everything out of the way...

Assuming lightbox and jQuery are loaded beforehand, you can have an implementation as little as this:

$.fn.alwaysVisibleLink = function (config) {
  return this.each(function () {
    $('<a/>', {
      href: config.href,
      class: 'cleanslate custom widgetExternalPage'
    })
      .text(config.text)
      .appendTo(this)
      .css({
        'position': 'fixed',
        'right': '0px',
        'top': '50%',
        'z-index': 1000000000,
        '-webkit-transform': 'rotate(270deg)',
        '-moz-transform': 'rotate(270deg)',
        '-o-transform': 'rotate(270deg)',
        '-ms-transform': 'rotate(270deg)',
        'transform': 'rotate(270deg)',
        'filter': 'progid:DXImageTransform.Microsoft.BasicImage(rotation=3)'
      })
      .colorbox({
        iframe: true,
        width: '80%',
        height: '80%'
      });
  });
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ If you refer to Google JS Map API(maps.googleapis.com/maps/api/js?v=3.exp&sensor=false), it assume user to put just couple of lines before closing <body> tag and then dynamically load all additional scripts files. I can't ask user to manually add dependency scripts as its a paid service (Feedback widget.) Other players like WebEngage also provide script where user just need to put couple of lines and script takes care of loading all the dependencies - Check out - blog.webengage.com/2012/04/28/… \$\endgroup\$
    – Ankur
    Commented Jan 8, 2014 at 15:26
  • \$\begingroup\$ @user3133397 The code you gave is not as big as Google API (And yes, I know how Google loads their scripts). You only mentioned "What widget does: It adds an always visible text on the screen." and that's about it. What you give is what you get. \$\endgroup\$
    – Joseph
    Commented Jan 8, 2014 at 15:29
  • \$\begingroup\$ Yes, I agree that its no where as big as Google Map API, but ain't it the review section? I believe (and correct me if I'm wrong) whole purpose of having this section is to get the code reviewed by peers and follow best practice. All I want is someone to comment on points that I mentioned in initial post and comment if anything can be done a better way. \$\endgroup\$
    – Ankur
    Commented Jan 8, 2014 at 15:40
  • \$\begingroup\$ @user3133397 Not all best practices apply to all cases. And in your case, keeping it small and simple is a better way. \$\endgroup\$
    – Joseph
    Commented Jan 8, 2014 at 15:49

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.