Library for material design toasts

Question

I wrote a small JavaScript library for material design toasts (notifications). Here's the GitHub repo.

I'd like to know what my mistakes are and how I can correct them. The code works correctly and was linted with jshint, but I'd like to know more about the code's level of quality. I'm interested in notes on readability and performance.

(function () {

function MadtarasToast() {
    var localToastConfig = {
        'duration': 5000,
        'style': {}
    };
    var currentToast;

    function show(toastConfig) {
        var currentToastId;

        // delete currentToast if it exist
        if (currentToast) {
            deleteCurrentToast();
        }

        // creating currentToast
        currentToast = document.createElement('div');
        ///currentToastId = 'madtaras-toast' + guid();
        currentToast.id = 'madtaras-toast' + guid();
        currentToastId = currentToast.id;
        currentToast.className = 'madtaras-toast __singleline';

        // choosing styles depending on number of symbols in toast
        if (toastConfig.actionInnerText) {
            var numOfSymbolsInToast = toastConfig.innerText.length +
              toastConfig.actionInnerText.length;
            currentToast.classList.add(numOfSymbolsInToast < 76 ?
              '__singleline' : '__multiline' );
        } else {
            currentToast.classList.add(toastConfig.innerText.length < 83 ?
              '__singleline' : '__multiline' );
        }

        // inserting message
        currentToast.insertAdjacentHTML('afterbegin',
          '<span class="madtaras-toast_text">' + toastConfig.innerText + '</span>');
        if (toastConfig.actionInnerText && toastConfig.actionCallback) {
            currentToast.insertAdjacentHTML('beforeend',
              '<span class="madtaras-toast_action-btn" id="madtaras-toast_action-btn">' +
              toastConfig.actionInnerText + '</span>');
            currentToast.querySelector('#madtaras-toast_action-btn').
              addEventListener('click', function () {
                  hide(currentToastId);
                  toastConfig.actionCallback();
              });
        }

        // applying styles from localToastConfig
        for ( var styleProperty in localToastConfig.style ) {
            if ( localToastConfig.style.hasOwnProperty(styleProperty) ) {
                currentToast.style[styleProperty] = localToastConfig.style[styleProperty];
            }
        }

        // inserting into body and start animating
        document.body.appendChild(currentToast);
        currentToast.classList.add('__fade-in');

        // setting timeout to hide toast
        setTimeout(function () {
            hide(currentToastId);
        }, toastConfig.duration || localToastConfig.duration);
    }

    function hide(toastToHideId) {
        var toastToHide = document.getElementById(toastToHideId);
        if ( !toastToHide ) return;
        toastToHide.addEventListener('webkitAnimationEnd', deleteCurrentToast);
        toastToHide.addEventListener('animationend', deleteCurrentToast);
        currentToast.classList.add('__fade-out');
    }

    // helping function
    function deleteCurrentToast() {
        currentToast.remove();
        currentToast = null;
    }

    // function to generate unique id for toast
    function guid() {
        function s4() {
            return Math.floor((1 + Math.random()) * 0x10000)
              .toString(16)
              .substring(1);
        }
        return s4() + s4() + '-' + s4() + '-' + s4() + '-' +
          s4() + '-' + s4() + s4() + s4();
    }

    // function to change localToastConfig
    function changeConfig(properties) {

        // changing default toast styles
        if ( properties.hasOwnProperty('style') ) {
            for ( var styleProperty in properties.style ) {
                if ( properties.style.hasOwnProperty(styleProperty) ) {
                    localToastConfig.style[styleProperty] = properties.style[styleProperty];
                }
            }
        }

        // changing other default properties
        for (var property in properties) {
            if ( properties.hasOwnProperty(property) && property !== 'style') {
                localToastConfig[property] = properties[property];
            }
        }
    }

    this.show = show;
    this.changeConfig = changeConfig;
}

window.madtarasToast = new MadtarasToast();

})();

Answer 1

I think you should work on splitting things up into even smaller functions.

Especially the parts in show where you start getting into the HTML.

    // choosing styles depending on number of symbols in toast
    if (toastConfig.actionInnerText) {
        var numOfSymbolsInToast = toastConfig.innerText.length +
          toastConfig.actionInnerText.length;
        currentToast.classList.add(numOfSymbolsInToast < 76 ?
          '__singleline' : '__multiline' );
    } else {
        currentToast.classList.add(toastConfig.innerText.length < 83 ?
          '__singleline' : '__multiline' );
    }

That can be a function determineLineCardinality, where you determine singleline or multiline. Maybe it doesn't even add to the class list, it just gives you the class name.

    // inserting message
    currentToast.insertAdjacentHTML('afterbegin',
      '<span class="madtaras-toast_text">' + toastConfig.innerText + '</span>');
    if (toastConfig.actionInnerText && toastConfig.actionCallback) {
        currentToast.insertAdjacentHTML('beforeend',
          '<span class="madtaras-toast_action-btn" id="madtaras-toast_action-btn">' +
          toastConfig.actionInnerText + '</span>');
        currentToast.querySelector('#madtaras-toast_action-btn').
          addEventListener('click', function () {
              hide(currentToastId);
              toastConfig.actionCallback();
          });
    }

That can be a function insertToastMessage and insertToastButton.

    // applying styles from localToastConfig
    for ( var styleProperty in localToastConfig.style ) {
        if ( localToastConfig.style.hasOwnProperty(styleProperty) ) {
            currentToast.style[styleProperty] = localToastConfig.style[styleProperty];
        }
    }

This... it's kind of a separate function, but it's only so big because you have to do some filtering. I guess it can stay.

        for ( var styleProperty in properties.style ) {
            if ( properties.style.hasOwnProperty(styleProperty) ) {
                localToastConfig.style[styleProperty] = properties.style[styleProperty];
            }
        }

But maybe you'd benefit from writing some abstract function which "overwritesExistingValues"? You're doing the same thing twice already...

Lastly, back over here...

        currentToast.classList.add(numOfSymbolsInToast < 76 ?
          '__singleline' : '__multiline' );
    } else {
        currentToast.classList.add(toastConfig.innerText.length < 83 ?
          '__singleline' : '__multiline' );

You have these "magic numbers", where apparently, as values 76 or 83, is some character limit. Maybe you should define this as "toastSingleLineLimit", and check against that? How did you even get to this symbol count? Do you use a monospace font? What if my text is "mmmmmmmmmm", or "iiiiiiiiii"? Then a non-monospace font may end up multilining too soon or too late. Consider finding another way to deal with the issue, if possible.

Answer 2

The thing is a singleton. I see little benefit of using a constructor. Try using an object literal instead. Something like:

;(function(namespace){

  namespace.show = function(){...};
  namespace.changeConfig = function(){...};

}(this.madtarasToast = this.madtarasToast || {});

I've seen this GUID generator in StackOverflow, which is neat. However, the s4 function might be generated everytime you call guid. Best you move it out of the function to avoid that. You still have the safety of your IIFE anyways.

;(function(namespace){

  function s4(){...}
  function guid(){...}

  namespace.show = function(){...};
  namespace.changeConfig = function(){...};

}(this.madtarasToast = this.madtarasToast || {});

Personal preference, but I usually collapse one-liner, very short conditionals into one line:

if(currentToast) deleteCurrentToast();

I notice that you often do checks for existence of config values like if (toastConfig.actionInnerText) and if (toastConfig.actionInnerText && toastConfig.actionCallback) and even toastConfig.duration || localToastConfig.duration. Instead of checking their existence every time, why not create a config, merge in the defaults and then the actual values? That way, you're always sure the value is there.

You can also do the same with changeConfig

var defaults = {...};
var config   = {...};

// The old and trusted way:
for(var key in defaults){
  // If it exists in defaults but not in config, fill it in
  if(defaults.hasOwnProperty(key) && !config.hasOwnProperty(key){
    config[key] = defaults[key];
  }
}

// ES6 way:
Object.assign({}, defaults, config);

I see huge chunks of code, especially ones in // creating currentToast and the few lines after it. If they're related blocks of code, it's best you move them out to a properly named function. That way, the intention is well described.

And lastly, commented out code is dead code. It's dead weight. Remove them from your code. You use a version control system, you have history. You will never lose them, you can always just roll back.

Answer 3

• Instead of: currentToast.classList.add(numOfSymbolsInToast < 76 ? '__singleline' : '__multiline' );, you can measure the height of the toast and apply the correct style.
• Why use double underscores in your class names?
• Don't use IDs, your code is being used on a page where you can't guarantee the ID won't exist. You already have references to currentToast and you could easily set a handler on the button without using an ID. Probably by using event delegation on currentToast
  • It seems like you can show two toasts at once, you will have a bug if you click the button on the second toast.
• Typically, you pass the global object into the module instead of accessing window directly: (function(global){ global.myModule = ...;})(this); Not really a problem here because your module depends on the DOM, but it's good to get into the practice

Answer 4

Suggestion for guid function:

/**
 * Generates a random UUID (v4).
 * @return {string} Returns generated random UUID.
 * @link http://en.wikipedia.org/wiki/Universally_unique_identifier
 * @link http://en.wikipedia.org/wiki/Globally_unique_identifier
 * @link http://www.ietf.org/rfc/rfc4122.txt
 * @static
 */
function guid() {
  return 'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {
    /** @type {number} */ var r = Math.random() * 16 | 0;
    return ('x' == c ? r : (r & 0x3 | 0x8)).toString(16);
  });
}

Source: https://github.com/Datamart/Glize/blob/master/src/util/StringUtils.js#L68