Support monthly indicators in line graphs #107
Conversation
| @@ -132,6 +131,12 @@ export class LineGraphComponent { | |||
| // Time scales only recognize annual and daily data | |||
| var parseTime = D3.timeParse(this.timeFormat); | |||
| this.extractedData.forEach(d => d.date = parseTime(d.date)); | |||
|
|
|||
| // Sort data by date ascending | |||
| this.extractedData.sort(function(a, b) {return a.date - b.date;}); | |||
There was a problem hiding this comment.
This creates a type checking error:
[default] /home/local/AZVA-INT/khoekema/projects/climate/climate-change-lab/src/app/charts/line-graph.component.ts:136:56
The left-hand side of an arithmetic operation must be of type 'any', 'number' or an enum type.
To me it seems like the problem goes back a little ways. I.e.
- The
DataPointtype defines itsdateattribute as a string here - But the
LineGraphComponentoverwrites it with a javascript Date object here
That seems like a bad situation. If we want dates, we should convert much earlier (e.g. here where the data is initially loaded). That would require moving the config that's used to parse and interpret the date to somewhere where it can be shared, but that might be a good thing anyway. Anything that's using DataPoint values as defined (i.e. treating the dates as strings) would have to be checked/revised to handle Date objects.
If we mostly want to keep the date keys as returned by the API (i.e. 'YYYY', 'YYYY-MM', or 'YYYY-MM-DD' strings) then we should define a separate data type for use inside LineGraphComponent and explicitly convert the data to that format. (Also, setLineScales seems like a weird place to mutate the data in a way that affects all subsequent steps. Doing it in filterData where the extractedData object is created would make more sense to me.)
There was a problem hiding this comment.
I see your point. There isn't a reason to maintain the string format of date that comes from the API. It makes sense to process date in the service while we are chopping up the data.
I could define two types: RawDataPoint and DataPoint. Former, string date; Latter, date date.
fungjj92
force-pushed
the
feature/SupportMonthlyIndicators
branch
from
43a9729 to
64e145d
Compare
|
Updated. I am now processing the strings into dates in chart.service and we still only have 1 type, DataPoint which now expects a Date date not a string date. Should be no more linter errors. |
| var parseTime = D3.timeParse(this.timeFormat); | ||
| this.extractedData.forEach(d => d.date = parseTime(d.date)); | ||
| // Sort data by date ascending | ||
| this.extractedData.sort(function(a, b) {return +a.date - +b.date;}); |
There was a problem hiding this comment.
Me: That seems like a kludge, can we do better?
The Google: microsoft/TypeScript#2361 (comment)
Me: 😱
Coercion it is!
Overview
Line graphs weren't set up to take monthly indicator data. Now they are!
Demo
Closes #105