ONSvisual / Simple-charts

Simple responsive charts
17 stars 12 forks source link

Improve vertical positioning of wrapped text #94

Open jtrim-ons opened 3 years ago

jtrim-ons commented 3 years ago

For example, in the bump chart (I've made the label for the United States longer to show how it looks when wrapped):

It might be nicer to centre the text like this:

This is the modified wrap() function for the bump chart:

function wrap(text, width) {
  text.each(function(d,i) {
    var text = d3.select(this),
        words = text.text().split(/\s+/).reverse(),
        word,
        line = [],
        lineNumber = 0,
        lineHeight = 1.1, // ems
        y = text.attr("y"),
        tspan = text.text(null).append("tspan").attr("x", 0).attr("y", y)
        tspans = [tspan];
    while (word = words.pop()) {
      line.push(word);
      tspan.text(line.join(" "));
      if (tspan.node().getComputedTextLength() > width) {
        line.pop();
        tspan.text(line.join(" "));
        line = [word];
        tspan = text.append("tspan").attr("x", 0).attr("y", y).text(word);
        tspans.push(tspan);
      }
    }
    var dy = -(tspans.length - 1) * lineHeight / 2;
    tspans.forEach(function(tspan) {
      tspan.attr("dy", dy + "em")
      dy += lineHeight;
    });
  });
}

Should I change the wrap functions in some of the templates?

henryjameslau commented 3 years ago

How does it handle 3 lines of wrapping?

jtrim-ons commented 3 years ago

Here's an example:

That's with the slightly tweaked version below. (It reduces lineHeight to 0.9 ems if there are three or more lines.)

function wrap(text, width) {
  text.each(function(d,i) {
    var text = d3.select(this),
        words = text.text().split(/\s+/).reverse(),
        word,
        line = [],
        lineNumber = 0,
        lineHeight = 1.1, // ems
        lineHeight3 = 0.9, // line height if there are 3 or more lines, ems
        y = text.attr("y"),
        tspan = text.text(null).append("tspan").attr("x", 0).attr("y", y)
        tspans = [tspan];
    while (word = words.pop()) {
      line.push(word);
      tspan.text(line.join(" "));
      if (tspan.node().getComputedTextLength() > width) {
        line.pop();
        tspan.text(line.join(" "));
        line = [word];
        tspan = text.append("tspan").attr("x", 0).attr("y", y).text(word);
        tspans.push(tspan);
      }
    }
    if (tspans.length >= 3) {
        lineHeight = lineHeight3;
    }
    var dy = -(tspans.length - 1) * lineHeight / 2;
    tspans.forEach(function(tspan) {
      tspan.attr("dy", dy + "em")
      dy += lineHeight;
    });
  });
}
henryjameslau commented 3 years ago

It works, it still feels a not complete solution to have something for 2 lines and something different for 3 lines although it's something we've done in the past

jtrim-ons commented 3 years ago

I know, it feels a bit ugly. I can't think of anything better though!

henryjameslau commented 3 years ago

Was wondering if you could do some .attr("transform","translate()"), maybe even on the parent text element?

jtrim-ons commented 3 years ago

Was wondering if you could do some .attr("transform","translate()"), maybe even on the parent text element?

Possibly. It might make the code more complicated though?

The decreased line height for 3 or more rows isn't really necessary. It just seemed to look nicer when you squeeze the lines closer together when there are lots of lines. Maybe there could just be a single lineHeight option in the config that you could play with until the chart looks right?

jtrim-ons commented 2 years ago

Just for reference, not necessarily the best way to do it: https://observablehq.com/@jtrim-ons/svg-text-wrapping