4
\$\begingroup\$

The task is creating a burn-up chart from Tickets on Trello

We make use of the Trello-API in C#, to get the tickets (Done/Created/etc...) however that is not what I want reviewed, since this has been done mostly before I joined this project. However I am a bit worried about my JavaScript for the d3.js code I created.

Code

<!DOCTYPE html>
<meta charset="utf-8">
<title>d3 burn chart</title>
<div id="charts"></div>

<style>
    div.tooltip {
        position: absolute;
        text-align: center;
        width: 100px;
        height: 32px;
        padding: 2px;
        font: 12px sans-serif;
        background: lightsteelblue;
        border: 0px;
        border-radius: 8px;
        pointer-events: none;
    }
</style>

<script src="https://d3js.org/d3.v4.min.js"></script>
<script>
    const burnuplist = [
        { Date: "/Date(1476079569717)/", Total:1, Burn:0 },
        { Date: "/Date(1476684369717)/", Total:23, Burn:2 },
        { Date: "/Date(1477289169717)/", Total:32, Burn:17 },
        { Date: "/Date(1477897569717)/", Total:57, Burn:40 },
        { Date: "/Date(1478502369717)/", Total:74, Burn:56 }
      ]

    var formatTime = d3.timeFormat("%d-%m-%Y");
    var drawLine = function(xvalues, yvalues, g, xscale, yscale, options) {
        // Define the div for the tooltip
        var div = d3.select("body").append("div")
            .attr("class", "tooltip")
            .style("opacity", 0);

        // line parts
        const lineparts = yvalues
            .map((y, i, ar) => ({
                i : i,
                x1: xvalues[i - 1],
                x2: xvalues[i],
                y1 : ar[i - 1],
                y2 : y
            }))
            .filter(d => d.i > 0);

        // line
        g.append("g")
            .attr("id", options.id) 
            .selectAll("line")
            .data(lineparts)
            .enter()
            .append("line")
            .attr("x1", d => xscale(d.x1))
            .attr("x2", d => xscale(d.x2))
            .attr("y1", d => yscale(d.y1))
            .attr("y2", d => yscale(d.y2))
            .attr("stroke", options.color);

        if (options.circles) {
            g.append("g")
                .selectAll("circle")
                .data(lineparts)
                .enter()
                .append("circle")
                .attr("r", 5)
                .attr("cx", d => xscale(d.x2))
                .attr("cy", d => yscale(d.y2))
                .attr("fill", options.color)
                .on("mouseover", function (d) {
                    div.transition()
                        .duration(200)
                        .style("opacity", .9);
                    div.html(d.y2 + "</br>" + d.x2)
                        .style("left", (d3.event.pageX) + "px")
                        .style("top", (d3.event.pageY - 28) + "px");
                })
                .on("mouseout", function (d) {
                    div.transition()
                        .duration(500)
                        .style("opacity", 0);
                });
        }
    }

    var burnchart = function(xlabels, burnlist, totallist) {
        const svg = d3.select('svg'),
            margin = {top: 20, right: 40, bottom: 75, left: 50},
            width = +svg.attr("width") - margin.left - margin.right,
            height = +svg.attr("height") - margin.top - margin.bottom,
            g = svg.append("g").attr("transform", `translate(${margin.left},${margin.top})`);

        // x-axis
        const x = d3.scalePoint()
            .domain(xlabels)
            .range([0, width]);
        g.append("g")
            .attr("transform", `translate(0,${height})`)
            .call(d3.axisBottom(x))
            .selectAll("text")
            .style("text-anchor", "end")
            .attr("transform", "rotate(-65)")
            .attr("y", 4)
            .attr("x", -10)
            .attr("dy", ".35em");

        // y-axis
        const y = d3.scaleLinear()
            .domain([0, 1.05 * Math.max(d3.max(burnlist), d3.max(totallist))])
            .range([height, 0])
            .nice();
        g.append("g")
            .call(d3.axisLeft(y));

        // total
        drawLine(xlabels, totallist, g, x, y,
            {
                color: "#000000",
                circles: true,
                id: "totallist"
            });

        // burn
        drawLine(xlabels, burnlist, g, x, y,
            {
                color: "#3c763d",
                circles: true,
                id: "burnlist"
            });
    }

    d3.select('#charts')
        .selectAll('svg')
        .data([burnuplist])
        .enter()
        .append('svg')
        .attr('width', 860)
        .attr('height', 480)
        .style('margin', '0.5em')
        .style('border', 'solid 2px #eef6fc')
        .call(burnchart(
                burnuplist.map(d => formatTime(new Date(parseInt(d.Date.replace("/Date(", "").replace(")/", ""), 10)))),
                burnuplist.map(d => d.Burn),
                burnuplist.map(d => d.Total)
            )
        );
</script>

Result

burn-up

Questions

  • Am I using correctly?
  • I somehow needed to make an array over the Json data [burnuplist]
  • Maybe out of scope, but is there a better way to change how I handle the Date's?
  • Any general tips on JavaScript styling?

I don't write much JS, as you can see ;) Any review is welcome.

Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

First of all, your code is working, which is a good thing. However... for any seasoned D3 developer your code is somewhat weird: it's not idiomatic and it has some strange patterns (which will make your life harder in the future).

Here I'll deal with your 4 bullet-point questions together, since they are deeply interconnected.

So, let's first see the major problems:

  1. You're binding the data to the SVG in an enter selection, but you never use it. So, just append a single SVG, without binding any data to it.
  2. Drop the burnChart and the drawLine functions. If you want to create a single drawing function, which is a good approach (regarding DRY), pass just the changing data to it, avoiding repetitions (the scales are always the same, for instance). But in a chart like this one such pattern is unnecessary (have a look at the each() in my last snippet).
  3. You're binding a single array of numbers both to the lines and to the circles. Instead of that, just filter the burnuplist array accordingly (or create two separate data arrays, as I'm doing here), and bind the whole array of objects to the circles and the path (which brings us to the next point). That way, each element has a datum with all properties in the object, that we can access easily. More importantly, that way you don't rely on the index of each element in the array as you're doing right now and which is an inadequate approach.
  4. Don't append a bunch of <line> elements to create a line chart. D3 has line generators that you can easily use to create a <path> element: the whole line will be a single SVG path. This is way more convenient, and on top of that you can use curves to smooth the path.
  5. You have time in the x axis. However, you're converting the date objects to strings and using a point scale with those strings as the domain, which is probably not the best practice. Treat time as time, using a time scale, unless you really want to treat each individual moment as a categorical variable (for instance, when the time span doesn't matter and you want the space between the ticks to be the same, regardless the actual time).

Some minor problems:

  1. Name all your selections. It's easier if you want to use them in the future.
  2. Give meaningful name to your variables, like xScale, not just x.
  3. You're mixing const and var, which may seem strange fome some people, as well as arrow functions with regular functions.
  4. Use lowercase for the properties: date instead of Date, total instead of Total etc...

All that being said, this is my version of your code. It's a major refactor (actually, I wrote it from scratch).

var svg = d3.select('#charts')
  .append('svg')
  .attr('width', 860)
  .attr('height', 480)
  .style('margin', '0.5em')
  .style('border', 'solid 2px #eef6fc');

var div = d3.select("#charts").append("div")
  .attr("class", "tooltip")
  .style("opacity", 0);

var burnuplist = [{
  Date: "/Date(1476079569717)/",
  Total: 1,
  Burn: 0
}, {
  Date: "/Date(1476684369717)/",
  Total: 23,
  Burn: 2
}, {
  Date: "/Date(1477289169717)/",
  Total: 32,
  Burn: 17
}, {
  Date: "/Date(1477897569717)/",
  Total: 57,
  Burn: 40
}, {
  Date: "/Date(1478502369717)/",
  Total: 74,
  Burn: 56
}];

var margin = {
    top: 20,
    right: 40,
    bottom: 75,
    left: 50
  },
  width = +svg.attr("width") - margin.left - margin.right,
  height = +svg.attr("height") - margin.top - margin.bottom,
  g = svg.append("g").attr("transform", `translate(${margin.left},${margin.top})`);

var formatTime = d3.timeFormat("%d-%m-%Y");

burnuplist.forEach(function(d) {
  d.Date = new Date(parseInt(d.Date.replace("/Date(", "").replace(")/", ""), 10))
});

var totalData = burnuplist.map(function(d) {
  return {
    date: d.Date,
    value: d.Total
  }
});

var burnData = burnuplist.map(function(d) {
  return {
    date: d.Date,
    value: d.Burn
  }
});

var xScale = d3.scaleTime()
  .range([0, width])
  .domain(d3.extent(burnuplist, function(d) {
    return d.Date
  }));


var yScale = d3.scaleLinear()
  .range([height, 0])
  .domain([0, d3.max(burnuplist, function(d) {
    return d.Total
  }) * 1.05]);

var lineGenerator = d3.line()
  .x(function(d) {
    return xScale(d.date)
  })
  .y(function(d) {
    return yScale(d.value)
  });

var gX = g.append("g")
  .attr("transform", `translate(0,${height})`)
  .call(d3.axisBottom(xScale).tickFormat(function(d) {
    return formatTime(d)
  }).tickValues(burnuplist.map(function(d) {
    return d.Date
  })))
  .selectAll("text")
  .style("text-anchor", "end")
  .attr("transform", "rotate(-65)")
  .attr("y", 4)
  .attr("x", -10)
  .attr("dy", ".35em");

var gY = g.append("g")
  .call(d3.axisLeft(yScale));

var totalLine = g.append("path")
  .datum(totalData)
  .attr("d", lineGenerator)
  .style("fill", "none")
  .style("stroke", "#000");

var burnLine = g.append("path")
  .datum(burnData)
  .attr("d", lineGenerator)
  .style("fill", "none")
  .style("stroke", "#3c763d");

var totalCircles = g.selectAll(null)
  .data(totalData)
  .enter()
  .append("circle")
  .attr("r", 5)
  .attr("cx", function(d) {
    return xScale(d.date)
  })
  .attr("cy", function(d) {
    return yScale(d.value)
  })
  .style("fill", "#000");

var burnCircles = g.selectAll(null)
  .data(burnData)
  .enter()
  .append("circle")
  .attr("r", 5)
  .attr("cx", function(d) {
    return xScale(d.date)
  })
  .attr("cy", function(d) {
    return yScale(d.value)
  })
  .style("fill", "#3c763d");

d3.selectAll("circle").on("mouseover", function(d) {
    div.transition()
      .duration(200)
      .style("opacity", .9);
    div.html("Value: " + d.value + "</br>Date: " + formatTime(d.date))
      .style("left", (d3.event.pageX) + "px")
      .style("top", (d3.event.pageY - 28) + "px");
  })
  .on("mouseout", function(d) {
    div.transition()
      .duration(500)
      .style("opacity", 0);
  });
div.tooltip {
  position: absolute;
  text-align: center;
  width: 100px;
  height: 32px;
  padding: 2px;
  font: 12px sans-serif;
  background: lightsteelblue;
  border: 0px;
  border-radius: 8px;
  pointer-events: none;
}
<script src="https://d3js.org/d3.v4.min.js"></script>
<div id="charts"></div>

The totalLine, burnLine, totalCircles and burnCircles may seem a lot of repetition, but this is idiomatic D3. If it bothers you, just use an each() for DRY, like this:

var svg = d3.select('#charts')
  .append('svg')
  .attr('width', 860)
  .attr('height', 480)
  .style('margin', '0.5em')
  .style('border', 'solid 2px #eef6fc');

var div = d3.select("#charts").append("div")
  .attr("class", "tooltip")
  .style("opacity", 0);

var burnuplist = [{
  Date: "/Date(1476079569717)/",
  Total: 1,
  Burn: 0
}, {
  Date: "/Date(1476684369717)/",
  Total: 23,
  Burn: 2
}, {
  Date: "/Date(1477289169717)/",
  Total: 32,
  Burn: 17
}, {
  Date: "/Date(1477897569717)/",
  Total: 57,
  Burn: 40
}, {
  Date: "/Date(1478502369717)/",
  Total: 74,
  Burn: 56
}];

var margin = {
    top: 20,
    right: 40,
    bottom: 75,
    left: 50
  },
  width = +svg.attr("width") - margin.left - margin.right,
  height = +svg.attr("height") - margin.top - margin.bottom,
  g = svg.append("g").attr("transform", `translate(${margin.left},${margin.top})`);

var formatTime = d3.timeFormat("%d-%m-%Y");

burnuplist.forEach(function(d) {
  d.Date = new Date(parseInt(d.Date.replace("/Date(", "").replace(")/", ""), 10))
});

var totalData = burnuplist.map(function(d) {
  return {
    date: d.Date,
    value: d.Total
  }
});

var burnData = burnuplist.map(function(d) {
  return {
    date: d.Date,
    value: d.Burn
  }
});

var xScale = d3.scaleTime()
  .range([0, width])
  .domain(d3.extent(burnuplist, function(d) {
    return d.Date
  }));


var yScale = d3.scaleLinear()
  .range([height, 0])
  .domain([0, d3.max(burnuplist, function(d) {
    return d.Total
  }) * 1.05]);

var lineGenerator = d3.line()
  .x(function(d) {
    return xScale(d.date)
  })
  .y(function(d) {
    return yScale(d.value)
  });

var gX = g.append("g")
  .attr("transform", `translate(0,${height})`)
  .call(d3.axisBottom(xScale).tickFormat(function(d) {
    return formatTime(d)
  }).tickValues(burnuplist.map(function(d) {
    return d.Date
  })))
  .selectAll("text")
  .style("text-anchor", "end")
  .attr("transform", "rotate(-65)")
  .attr("y", 4)
  .attr("x", -10)
  .attr("dy", ".35em");

var gY = g.append("g")
  .call(d3.axisLeft(yScale));

var groups = g.selectAll(null)
  .data([totalData, burnData])
  .enter()
  .append("g")
  .each(function(d, i) {
    var line = d3.select(this).append("path")
      .datum(d)
      .attr("d", lineGenerator)
      .style("fill", "none")
      .style("stroke", i ? "#3c763d" : "#000");

    var circles = g.selectAll(null)
      .data(d)
      .enter()
      .append("circle")
      .attr("r", 5)
      .attr("cx", function(d) {
        return xScale(d.date)
      })
      .attr("cy", function(d) {
        return yScale(d.value)
      })
      .style("fill", i ? "#3c763d" : "#000");
  })

d3.selectAll("circle").on("mouseover", function(d) {
    div.transition()
      .duration(200)
      .style("opacity", .9);
    div.html("Value: " + d.value + "</br>Date: " + formatTime(d.date))
      .style("left", (d3.event.pageX) + "px")
      .style("top", (d3.event.pageY - 28) + "px");
  })
  .on("mouseout", function(d) {
    div.transition()
      .duration(500)
      .style("opacity", 0);
  });
  div.tooltip {
    position: absolute;
    text-align: center;
    width: 100px;
    height: 32px;
    padding: 2px;
    font: 12px sans-serif;
    background: lightsteelblue;
    border: 0px;
    border-radius: 8px;
    pointer-events: none;
  }
<script src="https://d3js.org/d3.v4.min.js"></script>
<div id="charts"></div>

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ This is really good! Actually this is my first encounter with the d3.js library. I was hoping for an answer like this, showing me where I did horribly wrong. \$\endgroup\$
    – Ludisposed
    Jan 26, 2018 at 13:08
  • \$\begingroup\$ @Ludisposed You didn't do horribly wrong... it works. It's just not idiomatic. When you use a library/language, the idiomatic use not only facilitates understanding codes and tutorials but also allows you to improve the code, adding complexity without things breaking apart. \$\endgroup\$
    – Gerardo Furtado
    Jan 26, 2018 at 13:15
  • \$\begingroup\$ For instance, every now and then I see people using loops (for, forEach, while...) to append elements in a D3 code. That may even work, but not only it can go wrong in several situations as also it hinders the proper learning of D3. \$\endgroup\$
    – Gerardo Furtado
    Jan 26, 2018 at 13:36
2
\$\begingroup\$

Since Gerardo has already given a great review and focused on d3 code, I would like to focus on the Javascript code.

Abstracting opacity setting code - D.R.Y.

Per the D.R.Y. principle, the code to set the opacity on the tooltip could be abstracted to a function which accepts the duration and opacity. Building on Gerardo's advice, the tooltip element could also have a more appropriate name, like tooltipContainer (unless you're into that whole brevity thing... then maybe just tooltip, though that could be confusing with the CSS class).

var tooltipContainer = d3.select("body").append("div")
        .attr("class", "tooltip")
        .style("opacity", 0);

Gerardo pointed out the mix of const and var usage. If you wanted to keep the strict usage of const then that container could be declared using const.

Then after that, a function can be defined that sets the opacity

var transitionTooltipToOpacity = function(duration, opacity) {    
    tooltipContainer.transition()
        .duration(duration)
        .style("opacity", opacity);
};

Then the mouseover and mouseout event handlers can utilize that function. The following lines:

.on("mouseover", function (d) {
                div.transition()
                    .duration(200)
                    .style("opacity", .9);
                div.html(d.y2 + "</br>" + d.x2)
                    .style("left", (d3.event.pageX) + "px")
                    .style("top", (d3.event.pageY - 28) + "px");
            })
            .on("mouseout", function (d) {
                div.transition()
                    .duration(500)
                    .style("opacity", 0);
            });

can be simplified to the following:

.on("mouseover", function (d) {
    transitionTooltipToOpacity(200, .9);
    tooltipContainer.html(d.y2 + "</br>" + d.x2)
        .style("left", (d3.event.pageX) + "px")
        .style("top", (d3.event.pageY - 28) + "px");
})
.on("mouseout", transitionTooltipToOpacity.bind(null, 500, 0));

Notice that the mouseout handler has a partial function, which is created using Function.bind(). That way, the function will be called with the duration and opacity values pre-set (and the other arguments that are passed to the event handlers like the current datum d, the index i, etc. are also passed to that function in this case).

CSS border

Bearing in mind that you mostly wanted feedback on the Javascript, I noticed that the stylesheet for the tooltip contains: border: 0px. I don't believe that is necessary, unless there is some other style declaration that would need to be overridden. If it is required, then one can omit the units (i.e. px) or use none. While it is over 9 years old (and about ems), this SO question seems relevant.

Improve this answer
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Good advice regarding the names. I myself use pretty long names, like var horizontalAxisTickValuesFiltered and so on.... sometimes too long! \$\endgroup\$
    – Gerardo Furtado
    Jan 26, 2018 at 9:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.