function addHandlers

Question

I'm experimenting with text editing with fabricjs objects and have the following code that works but that I'd like reviewed. My writing feels really clunky to me for some reason and as I'm new to handlers I'm sure what to change if anything?

function addHandler(id, fn, eventName) {
  document.getElementById(id)[eventName || 'onclick'] = function() {
    var el = this;
    if (obj = canvas.getActiveObject()) {
      fn.call(el, obj);
      canvas.renderAll();
    }
  };
}

function setStyle(object, styleName, value) {
  if (object.setSelectionStyles && object.isEditing) {
    var style = { };
    style[styleName] = value;
    object.setSelectionStyles(style);
  }
  else {
    object[styleName] = value;
  }
}

function getStyle(object, styleName) {
  return (object.getSelectionStyles && object.isEditing)
    ? object.getSelectionStyles()[styleName]
    : object[styleName];
}

// underline
addHandler('underline', function(obj) {
  var isUnderline = (getStyle(obj, 'textDecoration') || '').indexOf('underline') > -1;
  setStyle(obj, 'textDecoration', isUnderline ? '' : 'underline');
});

// bold
addHandler('bold', function(obj) {
  var isBold = (getStyle(obj, 'fontWeight') || '').indexOf('bold') > -1;
  setStyle(obj, 'fontWeight', isBold ? '' : 'bold');
});

// italic
addHandler('italic', function(obj) {
  var isItalic = (getStyle(obj, 'fontStyle') || '').indexOf('italic') > -1;
  setStyle(obj, 'fontStyle', isItalic ? '' : 'italic');
});

Answer 1

You should avoid setting your event handlers like that (by assigning a function to the onclick property) because you can only have a single function assigned to that property so anything else you assign to it will overwrite the previous event. Instead you should use addEventListener.

function addHandler(id, fn, eventName) {
  document.getElementById(id).addEventListener(eventName || 'click', function() {
    if (obj = canvas.getActiveObject()) {
      fn.call(this, obj);
      canvas.renderAll();
    }
  });
}

There is a line of code (i.e. var isUnderline = (getStyle(obj, 'textDecoration') || '').indexOf('underline') > -1;) which is repeated a lot. It is probably enough to justify giving it it's own function...

function hasStyle(obj, prop, value){
    return !!~(getStyle(obj, prop) || '').indexOf(value);
}

This saves you from having to put isUnderline into memory and then garbage collect it later..

addHandler('bold', function(obj) {
  setStyle(obj, 'fontWeight', hasStyle(obj, 'fontWeight', 'bold') ? '' : 'bold');
});

Finally, you could remove redundancy (for the D.R.Y. principle) by using an object..

var styles = {textDecoration: 'underline', fontWeight: 'bold', fontStyle: 'italic'};
for(let prop in styles){
    if(!styles.hasOwnProperty(prop)) continue;
    addHandler(styles[prop], function(obj) {
        setStyle(obj, styles[prop], hasStyle(obj, prop, styles[prop]) ? '' : styles[prop]);
    });
}