wearethoughtfox / amnesty-dp-2016

Maps for Amnesty International using D3
https://www.amnesty.org/en/latest/research/2016/04/death-sentences-executions-2015/
0 stars 2 forks source link

Timeline skips more than one year at a time #29

Closed robertocarroll closed 7 years ago

robertocarroll commented 8 years ago

Need to investigate what's going on here.

pauldwaite commented 8 years ago

Is this bug still reproducible?

robertocarroll commented 8 years ago

yeah. Just tried it on Chrome on a Mac and it doesn't even make it through a single cycle.

filter

pauldwaite commented 8 years ago

Gotcha — so the arrow on the timeline should be moving more smoothly, and we should see every year.

I suspect this might be because every time the timeline hits a new year, we call draw() to redraw the map, and it has to re-create the entire SVG world map. I added a console.time('drawMap') call in draw() just before this line:

var country = g.selectAll(".country").data(topo);

And then console.timeEnd('drawMap') just before this section:

if (dir === "rtl") {
  tooltipOffset = -20;
}

On my Mac, it takes 400–600 ms to draw the map, which I think is the length of time the timeline appears to pause for.

Obviously we need to update the map whenever the year changes to reflect the different data, but maybe there’s a way to do it without drawing the whole thing from scratch.

timeline-performance

pauldwaite commented 8 years ago

So — I think just changing the classes of the SVG <path> elements that represent the countries with their status colour achieves the same result as re-drawing the entire map, and is much quicker.

But I’m not 100% sure that it definitely achieves the same result — it looks the same to my eyes, but maybe I’m missing something.

pauldwaite commented 8 years ago

I’ve merged my change into the main branch. @robertocarroll: if you could have a look at some point and see if the timeline still seems to be working as it should, we can take it from there. I can reverse my changes if they’ve caused problems.

robertocarroll commented 8 years ago

This is certainly way more efficient and solves the timeline skipping issue. Only thing is now the info in the infoxbox and detailbox don't change with the timeline. So the year, the total executions on the infobox and the DP and execution figures on the detail. I guess we could call all the thing draw was doing without actually redrawing the map?

pauldwaite commented 8 years ago

@robertocarroll ah yes I see. I could break those bits out into separate functions, call them from draw(), and call them from changeYear(). I’ll take a look.

robertocarroll commented 7 years ago

Hi @pauldwaite Sorry to bother you after so long. I'm updating the map with new data and I noticed the slider is missing the start year. So 2007 is not showing. I had a look around, but I couldn't see anything in the last few commits that had changed. Any idea why it's not showing up?

pauldwaite commented 7 years ago

Hi @robertocarroll ! No worries at all, that sounds weird. I’ll take a look first thing tomorrow morning.

pauldwaite commented 7 years ago

Hm — I’ve got the latest code in the v2-all branch, and run it on my laptop. In Safari (10), Chrome (53) and Firefox (49), all the timeline labels seem to display fine, including 2007.

Are you still seeing the problem on your end @robertocarroll ?

narrow

wide

robertocarroll commented 7 years ago

How odd! 2007 is just not there (Chrome/Safari). It's not actually the markup (rather than there but not showing).

screencapture-192-168-1-144-8080-1475505393458

pauldwaite commented 7 years ago

Very odd. Is that screenshot from your laptop?

(I did notice that the gh-pages branch doesn’t have the timeline at all, but that’s not up-to-date if I understand correctly — https://wearethoughtfox.github.io/amnesty-dp-2016/.)

robertocarroll commented 7 years ago

Yeah. It's the v2-all branch. gh-pages is the thing which is live at the moment which wasn't as complex - no timeline/search/etc.

pauldwaite commented 7 years ago

Gotcha. So it’s your laptop on which the bug is visible?

robertocarroll commented 7 years ago

Yes. Also if you try playing the timeline with a country selects it, it all goes a bit odd - deselects the country and loses content in the popup. I'm not sure if this is due to introducing bad data or from a bug?

pauldwaite commented 7 years ago

Might be worth getting a third opinion on the missing 2007 label then.

I see what you mean about playing the timeline with a country selected. I tried it with the USA selected, and I noticed that for 2013 and 2014, the pop-up content comes back. (And for Brazil, the country name comes back for those years.)

However, looking at data/data.json, the USA seems to have data for all years except 2011.

robertocarroll commented 7 years ago

So if the detailbox is open, the changeYear function passes the yearData again to activateCountry (around line 702).

Something going wrong in activateCountry. I've added some console.logs and for some reason neither the if statement nor the or statement get called. Also it could be something to do with the way data_country.name__localised is called in ready() (around line 161) - that doesn't get called again in activateCountry so the template doesn't have any new data. I can't quite remember when this all went wrong though ...

robertocarroll commented 7 years ago

OK. I'm going to break out the missing 2007 to another issue. I think this one came from the more efficient way of updating the timeline (See above), but I didn't notice the way it didn't update the detailBox.

pauldwaite commented 7 years ago

I think this section of code is failing:

    // If d doesn't look like a country object, then it's probably a list of all country objects for the current year, passed in from the timeline's change event listener. We thus need to find just the data for the country currently displayed in the detail box.
    for (var i=0; i<d.length; i++) {
      if (d[i].id === selectedCountryId) {
        d = d[i];
        break;
      }
    }

Specifically, this bit:

d[i].id

The id property name is uppercase in the data, so this should be:

d[i].ID

Or maybe this:

d[i]["ID"]

Although if we’re not sure if the casing will always be consistent, we should make the check a bit more robust.

There are also some style problems with the popup when data isn’t present, but I can take a look at those too. Just popping off to a dance class but I should be able to fix this when I get back.

robertocarroll commented 7 years ago

Brilliant spot! I've updated the id in the data.