New time scale `ticks.mode` and `ticks.source` options #4507

simonbrunel · 2017-07-15T10:51:56Z

Edit: added ticks.source: 'data' option value in #4568
Edit: renamed scale.ticks.mode to scale.distribution in #4582

--

This PR is a proposal to replace #4364: it seems to provides exactly the same functionalities (and more) but doesn't introduce any new scale classes (timebase & timeseries), no code/options duplication and has a lower size impact (~1.2KB vs ~3.4KB). It's also more optimized (especially when querying pixel for a specific value) and support scattered points.

Introduced two new options:

ticks.source ('auto'|'labels'|'data'): auto generates "optimal" ticks based scale size and time options (current time implementation). labels generates ticks from the user given data.labels values.
~~ticks.mode~~ distribution ('linear'|'series'): series displays ticks at the same distance from each other, whatever the time value they represent, while linear displays them linearly in time: the distance between each tick represent the amount of time between their time values.

Fixes #4185

@etimberg @benmccann @IlyaBeliaev

etimberg · 2017-07-15T12:21:25Z

src/helpers/helpers.time.js

@@ -55,7 +55,7 @@ module.exports = function(Chart) {
 		var ticks = [];
 		if (options.maxTicks) {
 			var stepSize = options.stepSize;
-			var startTick = options.min !== undefined ? options.min : niceRange.min;
+			var startTick = options.min === undefined || options.min === null ? niceRange.min : options.min;


could use helpers.isNullOrUndef here (once rebased against that PR)

etimberg · 2017-07-15T12:24:19Z

src/scales/scale.time.js

 		}
 	};

+	function buildLookupTable(ticks, min, max, linear) {


This lookup table idea could probably be used as a better solution to handling 0 in the log axis. We would define a 0 point, and then the rest of it is a logarithmic interpolation region.

I think that the whole lookup (table) logic + ticks.mode/source options could be the base implementation for the linear, radiallinear, log and time scales (numeric scale?). This would refactor lot of code and make more consistent API between all that scales.

i agree. it would save a lot of code elsewhere to move it to the base class

Let's keep that work for another refactoring PR

benmccann · 2017-07-15T17:01:22Z

src/scales/scale.time.js

@@ -36,10 +40,94 @@ module.exports = function(Chart) {
 			},
 		},
 		ticks: {
-			autoSkip: false
+			autoSkip: false,
+			mode: 'series',   // 'linear|series'


I think series and auto is perhaps not the best default because I think that most of the time using series it'd probably be used with linear. I think auto will probably be the better option most of the time when linear is used. So I'd probably change on or the other of the defaults here.

linear is the current default on the timescale. I'm assuming linear & auto is supported eventhough there's no screenshot of that one. That'd be my vote for combination of options for the default

auto is the default since it the current time scale behavior, changing it will be a breaking change. auto generates linear ticks and in this case series or linear modes have exactly the same result. Now, let's say you change source for labels, as a user I would expect to have the series alignment of ticks by default. That why I picked series and auto by default.

auto doesn't exist in 2.6.0. It's been added since the most recent release. In fact it's a breaking change to switch it to auto since the only behavior that exists in 2.6.0 is linear

series & auto together is not doing something that makes a lot of sense to me. Perhaps we should disallow that combination of options. If auto is forcing it to be linear eventhough the user specified series that's going to be really confusing

auto is the actual time scale implementation. It generates linear ticks because that's the way how generateTicks works but we can imagine options, for example to exclude some ranges or whatever, that would make the generated ticks not linear anymore. In this case, auto + series would perfectly works and that's the reason I split it in 2 options since all combinations make sense.

By default having auto + series or auto + linear is the same as long as we don't change how generateTicks works. The question is: if the user switch to ticks.source: 'labels', what result it should expect by default? I think labels + series would be the most wanted choice (which is your timeseries behavior).

I also think that auto + series will be the best default when we will allow non linear ticks automatic generation: for example, let's say weekend are excluded, generated ticks would be: ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Monday", "Tuesday", ...] and we will want the same distance between each ticks (ticks.mode: 'series'). If you set ticks.mode: 'linear', the distance between each "Friday" and "Monday" will be 3 times the distance between other days.

I think auto + linear needs to be the default based on my understanding of series. When the mode is set to series, the physical spacing between the ticks in pixels is constant.

However, this may cause problems when the time.min and time.max settings come into play. When these settings are used, it is likely that the gap between the ticks is not constant.

Seems you right, auto + series and auto + linear don't do the same if leading / trailing times are adjusted. Still think auto + series would be better defaults but it's a breaking change, so should be: auto + linear.

Since unit tests pass whatever auto + series or auto + linear, could be great to have you guys testing use cases that could break. I didn't succeed to have different results between both combinations, even with explicit min/max.

benmccann · 2017-07-15T17:02:59Z

src/scales/scale.time.js

 		}
 	};

+	function buildLookupTable(ticks, min, max, linear) {
+		var ilen = ticks.length;


I think ilen isn't very understandable as a name. why not just use ticks.length everywhere? is it a minification thing? could we call it ticksLen instead?

No, it's for performance, caching the length of the array is faster depending on the JS engine. ilen, jlen, etc. is commonly use out there and since it's most of the time only used for that purpose, I prefer to not change it (I also like that it's short and also refer to the i var).

You don't need to set ilen to ticks.length both here and below. You could just declare ilen with all the other variables and then initialize below in the loop as you're doing now

Good catch, will keep it initialized at the top though since I'm using it at line 54.

benmccann · 2017-07-15T17:04:03Z

src/scales/scale.time.js

+			table.push({time: min, decimal: 0});
+		}
+
+		for (i=0, ilen=ticks.length; i<ilen; ++i) {


can we just declare i here instead of up above? can we declare the other variables where they're first used?

It's better for minification, gather all var declarations in the same block.

Also declaring var in loops can have performance impact on some JS engines.

benmccann · 2017-07-15T17:09:44Z

src/scales/scale.time.js

+
+			// only add points that breaks the scale linearity
+			if (Math.round((next+prev)/2) !== curr) {
+				decimal = linear? (curr - min) / (max - min) : ilen > 1? i/(ilen-1) : 0;


should probably add a space before the ? characters. we're also inconsistent about spacing around the / characters on this line

I think a set of parens here would help clarify order of operations. how about:

linear ? (curr - min) / (max - min) : (ilen > 1 ? i / (ilen-1) : 0);

(at least i'm assuming that's the order of operations and it isn't (linear ? (curr - min) / (max - min) : ilen > 1) ? i / (ilen-1) : 0;)

+1 for the space before ?, however I'm not in favor of useless parenthesis.

ok. works for me

benmccann · 2017-07-15T17:11:03Z

src/scales/scale.time.js

+
+			// If value is out of bounds, use ticks [0, 1] or [n-1, n] for interpolation,
+			// note that the lookup table always contains at least 2 items (min and max)
+			var prev = !range.lo? table[0] : !range.hi? table[table.length-2] : range.lo;


I think it's strange not to have a space before ? throughout

benmccann · 2017-07-15T17:12:42Z

src/scales/scale.time.js

 		}
 	};

+	function buildLookupTable(ticks, min, max, linear) {


it'd help to document what the key and value of this table are. i.e. what's being looked up

benmccann · 2017-07-15T17:15:15Z

src/scales/scale.time.js

+			max = parse(options.time.max, me) || max;
+
+			// In case there is no valid min/max, let's use today limits
+			min = min === MAX_INTEGER? +moment().startOf('day') : min;


what's the + on this line mean or do? i've never seen that syntax

http://momentjs.com/docs/#/displaying/unix-timestamp-milliseconds/

I'd prefer to be more explicit and just use .valueOf() here for readability. I'm sure I'm not the only one who wouldn't be familiar enough with moment to know about the + syntax

I'm not familiar with moment (might be the first time) and had to search in their docs what valueOf actually returns. The unary operator is commonly used to convert string to number (+'3' === 3) and I think it's not more confusing than valueOf once you know what it does. It may be even more obvious since you would expect a number from +moment() which is likely the timestamp. And it's also shorter, so better for minification.

benmccann · 2017-07-15T17:19:24Z

src/scales/scale.time.js

@@ -10,6 +10,10 @@ module.exports = function(Chart) {

 	var timeHelpers = helpers.time;


I originally refactored out helpers.time so that it could be shared between the time and timeseries scales. Should we move them back to this file if we're not going to have two scales?

Yeah totally agree, that's my next task with some optimizations if this PR get merged

benmccann · 2017-07-15T17:23:10Z

src/scales/scale.time.js

+	// @see adapted from http://www.anujgakhar.com/2014/03/01/binary-search-in-javascript/
+	function lookup(table, key, value) {
+		var lo = 0;
+		var hi = table.length -1;


spacing with operators is inconsistent throughout this function. e.g. there's a space before the - but not after

benmccann · 2017-07-15T17:33:59Z

src/scales/scale.time.js

-			return this.getPixelForOffset(this.ticksAsTimestamps[index]);
+			return index >= 0 && index < this.ticks.length?
+				this.getPixelForOffset(this.ticks[index].time) :
+				null;


I think eslint 4 will want each continued line to have extra indent. Can we just put this null on the previous line to avoid that error?

We didn't commit on that rule and ESLint can be tweaked to disable it. I tried to get used to it but I still prefer that form since it mimics the if/else indentation. Having null on the previous line doesn't make it more readable: should be 1 line or 3, not 2. And since having all in one line is too long, I definitely prefer this layout. That's really minor though.

simonbrunel · 2017-07-15T20:13:19Z

@etimberg @benmccann Not sure what you both think about this implementation, but if we are going to merge this PR instead of #4364, then I can start writing unit tests, docs and a sample that showcase the different combinations of source + mode.

benmccann · 2017-07-15T21:14:01Z

src/scales/scale.time.js

+			prev = ticks[i - 1] || 0;
+			curr = ticks[i];
+
+			// only add points that breaks the scale linearity


can you expand upon this either here or in the main docs for the lookup table. It's not immediately clear to me what the structure of this table is or what this logic is for

You right, I'm going to document a bit more the lookup stuff in the code.

benmccann · 2017-07-15T21:27:59Z

I'm very impressed you were able to combine the two classes into a single one with so few if/else checks to check the mode! So yeah, I think we should probably use this one as it seems more elegant.

I haven't had time to dig into the lookup table to really understand it yet. So I haven't read through the logic to check it's meeting all the different cases, but the screenshots all look like they're generating reasonable output, so I imagine it should be pretty close.

I'll definitely continue to test this out in the future especially as I try to migrate the financial chart to use this scale instead of the one in the financial repo. The only thing I wanted to ask about is whether we could have the dataset the user passes in use t or x instead of just x. The financial chart right now uses t which sort of makes more sense when you're specifying timestamps instead of just specifying cartesian coordinates.

simonbrunel · 2017-07-15T22:00:58Z

When ticks.mode: 'series', the scale can become non linear in time and it's not anymore possible to interpolate the requested time (t) between min and max to calculate its position (pixel). We need to find which ticks are before and after t, and interpolate their positions. However, iterating on all ticks to find the best matches, for every query, would be bad for performances.

This table is used to optimize queries from value -> pixel and pixel -> value (binary search). It contains only ticks that break the time linearity and each entry represents a time and a position. Initially, the position was in pixel but we need to build this table before the layout is fully done (no guaranty on left, top, width and height). That's why the position is between [0 1] (actually, I'm going to rename decimal to pos), 0 being the left (or top) of the scale and 1 the left+width (or top+bottom) (the final pixel position is computed in getPixelForOffset).

The worst case is when all ticks break the linearity, generating one item in the table for each tick. That's an extreme scenario, but the binary search makes it very fast. The best case is when all ticks are linear and so the table contains only 2 items (min and max), making lookups as fast as the current time scale implementation.

benmccann · 2017-07-16T18:11:01Z

Just a heads up, this PR needs a rebase now

benmccann · 2017-07-16T18:23:01Z

src/scales/scale.time.js

+	 * meaning that in the best case, it contains only 2 objects: {min, 0} and {max, 1}. `pos` is
+	 * a decimal between 0 and 1: 0 being the start of the scale (left or top) and 1 the other
+	 * extremity (left + width or top + height). Note that it would be more optimized to directly
+	 * store pre-computed pixels, but the scale dimensions are not guaranty at the time we need


guaranty -> guaranteed ?

benmccann · 2017-07-16T18:33:32Z

src/scales/scale.time.js

+	 * Returns an array of {time, pos} objects used to interpolate a specific `time` or position
+	 * (`pos`) on the scale, by searching entries before and after the requested value. Since the
+	 * value is linearly interpolated, only timestamps that break the time linearity are added,
+	 * meaning that in the best case, it contains only 2 objects: {min, 0} and {max, 1}. `pos` is


Could you at the beginning of this method do something like the below? (not sure this is exactly right, but should give the idea)

if (linear) { table.push({min, 0}); table.push({max, 1}); return table; }

Then you could simplify the section with the complicated logic a bit. Right now it's:

if (Math.round((next + prev) / 2) !== curr) { pos = linear ? (curr - min) / (max - min) : ilen > 1 ? i / (ilen - 1) : 0; table.push({time: curr, pos: pos}); }

I think it could just become:

table.push({time: curr, pos: ilen > 1 ? i / (ilen - 1) : 0});

I think this has a few readability advantages. First, it would make it pretty explicit from reading the code what the lookup table looks like in the linear case. Right now it's not as immediately obvious from the code. Second, I think you can make the clarifying assumption that every point will break linearity in the series case, which would also simplify things by removing the if check. There may be some points that don't break linearity, but I'm assuming it doesn't hurt to add extra points to the lookup table.

Edit: You may be right on the first part, simplify the linear table with min/max. I would keep the linearity check though since it's a great optim that applies to all sources when series.

Fixed, good catch, thanks! I was too much focused on the series approach that I didn't realize that we don't need the intermediary ticks when linear.

simonbrunel · 2017-07-16T20:25:01Z

Something I didn't test is if it works with vertical time scale.

simonbrunel · 2017-07-16T20:37:29Z

Squashed and rebased (and typo fixed).

`ticks.source` (`'auto'`(default)|`'labels'`): `auto` generates "optimal" ticks based on min, max and a few more options (current `time` implementation`). `labels` generates ticks from the user given `data.labels` values (two additional trailing and leading ticks can be added if min and max are provided). `ticks.mode` (`'linear'`(default)|`series`): `series` displays ticks at the same distance from each other, whatever the time value they represent, while `linear` displays them linearly in time: the distance between each tick represent the amount of time between their time values.

simonbrunel · 2017-07-19T09:57:16Z

@benmccann Can you have a look at my last commit: it fixes a few edge cases (zero or one tick) and takes in account the effective scale min and max to build the lookup table. It also makes sure that the lookup table always contains two items (at least min/max) which simplifies things.

benmccann · 2017-07-19T17:49:14Z

src/scales/scale.time.js

+		];
+	}
+
+	var table = [];


I'd move this declaration down to just before the for loop where it's first used to make it clearer that it's not used until then

Better for minification to have it gathered with other declarations.

benmccann · 2017-07-19T17:51:44Z

src/scales/scale.time.js

+	}
+
+	var table = [];
+	var items = timestamps.slice(0);


Maybe add a comment here like:

// construct an "items" array with min, timestamps, max in that order

I'm not enough of a JS expert to know what slice(0) and unshift do off the top of my head, so I had to Google it. The comment would just make it a little faster to read

I usually don't add this kind of comment since it explains what the code do and IMO slice(), unshift() and push() are kind of JavaScript basis, so reading the code should be enough to understand it. But since you ask for it, will add a note.

Actually, it's not even an array with min, timestamps and max, it's an array with min if min !== timestamps[0], timestamps, max if max !== timestamps[length -1]. It would be a comment that explains exactly the code, so I prefer not.

benmccann · 2017-07-19T18:00:15Z

src/scales/scale.time.js

+
+	for (i = 0, ilen = items.length; i<ilen; ++i) {
+		next = items[i + 1] || 0;
+		prev = items[i - 1] || 0;


this || 0 gives me pause

Let's say you have:

i = 0

prev = 0

curr = 2

next = 4

Or you have:

i = 0

prev = 0

curr = 2

next = 5

Is it really the case that one of these should trigger the if statement and one shouldn't? I guess in actuality that's quite unlikely to happen, but it's probably slightly safer to leave the variable undefined and then add an extra check for that in the if statement

benmccann · 2017-07-19T18:01:02Z

src/scales/scale.time.js

+	var mid, i0, i1;
+
+	while (lo >= 0 && lo <= hi) {
+		mid = ((lo + hi) >> 1);


probably don't really need parens around (lo + hi) >> 1

benmccann · 2017-07-19T18:03:54Z

src/scales/scale.time.js

+	return table;
+}
+
+// @see adapted from http://www.anujgakhar.com/2014/03/01/binary-search-in-javascript/


it's probably a bit better to re-implement this as the author hasn't granted a license for its use. https://www.quora.com/Intellectual-Property-Law-Is-it-legal-to-copy-small-code-snippets-from-open-source-software

I think it's fine, our version is a bit different (not a copy/paste) and it's a binary search of a range and not an exact value. If I had to re-implement, I would do it exactly the same way since it's pretty clean and efficient.

benmccann · 2017-07-19T18:20:25Z

src/scales/scale.time.js

-				maxTimestamp = timeHelpers.parseTime(me, timeOpts.max).valueOf();
+			} else {
+				stepSize = helpers.valueOrDefault(timeOpts.stepSize, timeOpts.unitStepSize);
+				stepSize = stepSize || timeHelpers.determineStepSize(min, max, unit, capacity);


You could probably make these a single statement:

stepSize = helpers.valueOrDefault(timeOpts.stepSize, timeOpts.unitStepSize) || timeHelpers.determineStepSize(min, max, unit, capacity);

benmccann · 2017-07-19T18:20:49Z

src/scales/scale.time.js

+				});
+
+				// Recompute min/max, the ticks generation might have changed them (BUG?)
+				min = ticks.length? ticks[0] : min;


space before ? here and the next line

benmccann · 2017-07-19T18:22:09Z

Looks good to me! I left a handful of really minor comments. Thanks for all the extra tests!

simonbrunel · 2017-07-28T17:06:42Z

@etimberg @benmccann what do you think to rename ticks.mode to ticks.distribute ('linear'|'series')? I think it's a much better name that represents exactly what this option does (mode being pretty vague).

etimberg · 2017-07-28T22:06:59Z

not opposed. maybe ticks.distribution. how does this interact with the other proposed distribute option?

simonbrunel · 2017-07-29T05:00:52Z

I think it represents very well what the option is but if you have other suggestions we can still iterate since it's not released yet. I'm totally fine with distribution (I usually prefer shorter names). The other proposed option is not directly related and I think distribute wasn't the best name ever for it. Maybe ticks.offset: true/false is better since it's supposed to replace the internal includeOffset logic.

`ticks.source` (`'auto'`(default)|`'labels'`): `auto` generates "optimal" ticks based on min, max and a few more options (current `time` implementation`). `labels` generates ticks from the user given `data.labels` values (two additional trailing and leading ticks can be added if min and max are provided). `ticks.mode` (`'linear'`(default)|`series`): `series` displays ticks at the same distance from each other, whatever the time value they represent, while `linear` displays them linearly in time: the distance between each tick represent the amount of time between their time values.

simonbrunel requested a review from etimberg July 15, 2017 10:51

etimberg reviewed Jul 15, 2017

View reviewed changes

simonbrunel force-pushed the time-scale-rewrite branch from 4dc75e1 to a8151b5 Compare July 15, 2017 13:19

benmccann reviewed Jul 15, 2017

View reviewed changes

simonbrunel force-pushed the time-scale-rewrite branch from c503732 to ca6170b Compare July 15, 2017 20:14

benmccann reviewed Jul 15, 2017

View reviewed changes

simonbrunel added this to the Version 2.7 milestone Jul 16, 2017

simonbrunel added the type: enhancement label Jul 16, 2017

benmccann reviewed Jul 16, 2017

View reviewed changes

simonbrunel force-pushed the time-scale-rewrite branch from 1046bc4 to c1458d9 Compare July 16, 2017 20:31

simonbrunel force-pushed the time-scale-rewrite branch 4 times, most recently from 2a82bc3 to dbeb5ea Compare July 18, 2017 07:37

simonbrunel force-pushed the time-scale-rewrite branch from dbeb5ea to b3fd40d Compare July 18, 2017 16:49

simonbrunel force-pushed the time-scale-rewrite branch from b3fd40d to 7c1a823 Compare July 18, 2017 20:43

etimberg approved these changes Jul 18, 2017

View reviewed changes

Enhance interpolation when one or zero tick

acd4146

benmccann reviewed Jul 19, 2017

View reviewed changes

Code review

8bd2422

simonbrunel merged commit 5ad1b4a into chartjs:master Jul 19, 2017

simonbrunel deleted the time-scale-rewrite branch July 19, 2017 19:04

This was referenced Jul 19, 2017

[FEATURE] Timeseries scale type #4189

Closed

Don't shift timescale bars on barchart #4063

Closed

This was referenced Jul 24, 2017

Unable to use time and category together #4185

Closed

New time scale ticks.source: 'data' option #4568

Merged

etimberg mentioned this pull request Jul 27, 2017

Last tick (grid column) extended. #4576

Closed

simonbrunel mentioned this pull request Aug 1, 2017

Change ticks.mode to scale.distribution #4582

Merged

benmccann mentioned this pull request Sep 19, 2017

Add support for ticks.source: 'data' to category scale #4776

Closed

simonbrunel mentioned this pull request May 24, 2018

Prefix private time scale methods with underscore #5512

Closed

		@@ -10,6 +10,10 @@ module.exports = function(Chart) {

		var timeHelpers = helpers.time;

New time scale ticks.mode and ticks.source options #4507

New time scale ticks.mode and ticks.source options #4507

Conversation

simonbrunel commented Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Jul 15, 2017 • edited

Choose a reason for hiding this comment

simonbrunel Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann Jul 15, 2017 • edited

Choose a reason for hiding this comment

simonbrunel Jul 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel commented Jul 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Jul 15, 2017

simonbrunel commented Jul 15, 2017 • edited

benmccann commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017 • edited

Choose a reason for hiding this comment

simonbrunel Jul 16, 2017 • edited

Choose a reason for hiding this comment

simonbrunel commented Jul 16, 2017

simonbrunel commented Jul 16, 2017

simonbrunel commented Jul 19, 2017

Choose a reason for hiding this comment

simonbrunel Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel Jul 19, 2017 • edited

Choose a reason for hiding this comment

simonbrunel Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Jul 19, 2017

New time scale `ticks.mode` and `ticks.source` options #4507

New time scale `ticks.mode` and `ticks.source` options #4507

simonbrunel commented Jul 15, 2017 •

edited

simonbrunel Jul 15, 2017 •

edited

simonbrunel Jul 15, 2017 •

edited

benmccann Jul 15, 2017 •

edited

simonbrunel Jul 15, 2017 •

edited

simonbrunel commented Jul 15, 2017 •

edited

simonbrunel Jul 16, 2017 •

edited

simonbrunel Jul 16, 2017 •

edited

simonbrunel Jul 19, 2017 •

edited

simonbrunel Jul 19, 2017 •

edited

simonbrunel Jul 19, 2017 •

edited

simonbrunel commented Jul 29, 2017 •

edited