HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Absolve me of my guilt: Dynamically creating spans that highlight on click

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
absolveclickdynamicallycreatingthatspanshighlightguilt

Problem

I'm working on a page that shows groups of users. The following JavaScript function is used to show more details about a user (such as email and login) when you click on their name.

But there's a twist: clicking on one of those pieces of information will highlight it to make it easier to copy. This is accomplished by adding an in-line onclick function to each span element containing the user info.

function showUserDetailsInRow(row, user) {
  row.innerHTML = "";
  var expanderCell = document.createElement("div");
  expanderCell.className = "GroupsCell";
  expanderCell.innerHTML = "-";

  var spanOpen = "",
   spanClose = "";

  var mainCell = document.createElement("div");
  mainCell.className = "GroupsCell";
  mainCell.innerHTML = "Name: " + spanOpen + user.displayName + spanClose + "" + (user.isDomainGroup ? "(Domain Group)" : ("Email: " + spanOpen + user.email + spanClose + "" + "Login: " + spanOpen + user.loginName + spanClose + ""));

  var removeCell = document.createElement("div");
  removeCell.className = "btn";
  removeCell.innerHTML = "remove from group";

  row.appendChild(expanderCell);
  row.appendChild(mainCell);
  row.appendChild(removeCell);
}


This works just as I'd expect, but it itches a bit.

-
I feel dirty putting code in a string. Is there a better way to accomplish this without greatly increasing the complexity of this function? Would it be worthwhile or more efficient to offload that dynamically repeated inline function into a single function definition elsewhere in my script instead?

-
I also feel guilty building the innerHTML of the mainCell element via string concatenation (and using tags like ` and
); should I instead be creating elements via document.createElement, decorating them with class names, and appending those to mainCell? Is there a benefit (whether it be performance, complexity, flexibility, or the like) to doing it one way or another?

Here's a working example of the
showUserDetailsInRow()` function,

Solution

In the order I spot things (so in no particular order):

  • Don't use innerHTML, especially not with user input. Imagine a user called Bobalert('I am evil');



  • You can use document.createElement() to create elements, document.createTextNode() to create a text node, or element.textContent = ... to set plain text.



  • Don't use explicit styling in your JavaScript. Give it a className and use CSS to style.



  • If you don't use string concatenation to create your elements, you can actually use a proper JavaScript function as the event handler.



  • Even better, you can probably make use of event delegation, since all the elements are of similar functionality.



You might also benefit the use of a templating engine such as Handlebars or Mustache.

Context

StackExchange Code Review Q#110019, answer score: 4

Revisions (0)

No revisions yet.