patternjavascriptMinor
Absolve me of my guilt: Dynamically creating spans that highlight on click
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.
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
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):
You might also benefit the use of a templating engine such as Handlebars or Mustache.
- Don't use
innerHTML, especially not with user input. Imagine a user calledBobalert('I am evil');
- You can use
document.createElement()to create elements,document.createTextNode()to create a text node, orelement.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.