Use line tile in place page #2734
Conversation
| @@ -51,6 +51,10 @@ interface LineTilePropType { | |||
| apiRoot?: string; | |||
| // Whether or not to render the data version of this tile | |||
| isDataTile?: boolean; | |||
| // The link of the stat var legend | |||
There was a problem hiding this comment.
nit: mention that it's a map where key is the stat var dcid and value is the link to use for that stat var
static/js/place/chart_block.tsx
Outdated
| ></Chart> | ||
| ); | ||
| } | ||
| const id = randDomId(); |
There was a problem hiding this comment.
this chart used to only be added to the page if there is trend data, but now it's always added to the page? I don't think we want to show empty charts on the place page?
There was a problem hiding this comment.
Right now Flask still fetches the place page cache (for bar data) and trim spec if no data is available. So only valid spec are returned, thus the line chart always have data.
A related issue is when line chart has single dot. I added a callback function to LineLine, which set "display: false" so the line tile is removed if there is only single dot.
static/js/place/chart.tsx
Outdated
| }; | ||
| })} | ||
| svgChartHeight={CHART_HEIGHT} | ||
| className={"ranking-class"} |
There was a problem hiding this comment.
curious what this class is for & what ranking has to do with this
There was a problem hiding this comment.
This should be removed.
static/js/place/chart.tsx
Outdated
| // TODO(datcom): handle i18n for scaled numbers | ||
| const scaling = this.props.scaling ? this.props.scaling : 1; | ||
| const sv = !_.isEmpty(this.props.statsVars) ? this.props.statsVars[0] : ""; | ||
| switch (this.props.chartType) { | ||
| case chartTypeEnum.LINE: |
There was a problem hiding this comment.
I think you also need to update the drawChart function & componentDidUpdate?
There was a problem hiding this comment.
Removed the drawLine() in drawChart
| @@ -51,6 +51,10 @@ interface LineTilePropType { | |||
| apiRoot?: string; | |||
| // Whether or not to render the data version of this tile | |||
| isDataTile?: boolean; | |||
| // The link of the stat var legend | |||
static/js/place/chart.tsx
Outdated
| }; | ||
| })} | ||
| svgChartHeight={CHART_HEIGHT} | ||
| className={"ranking-class"} |
There was a problem hiding this comment.
This should be removed.
static/js/place/chart.tsx
Outdated
| // TODO(datcom): handle i18n for scaled numbers | ||
| const scaling = this.props.scaling ? this.props.scaling : 1; | ||
| const sv = !_.isEmpty(this.props.statsVars) ? this.props.statsVars[0] : ""; | ||
| switch (this.props.chartType) { | ||
| case chartTypeEnum.LINE: |
There was a problem hiding this comment.
Removed the drawLine() in drawChart
static/js/place/chart_block.tsx
Outdated
| ></Chart> | ||
| ); | ||
| } | ||
| const id = randDomId(); |
There was a problem hiding this comment.
Right now Flask still fetches the place page cache (for bar data) and trim spec if no data is available. So only valid spec are returned, thus the line chart always have data.
A related issue is when line chart has single dot. I added a callback function to LineLine, which set "display: false" so the line tile is removed if there is only single dot.
| @@ -152,8 +169,7 @@ export function draw( | |||
| chartData.unit | |||
| ); | |||
| if (!isCompleteLine) { | |||
| svgContainer.querySelectorAll(".dotted-warning")[0].className += | |||
| " d-inline"; | |||
| elem.querySelectorAll(".dotted-warning")[0].className += " d-inline"; | |||
There was a problem hiding this comment.
maybe should check that querySelectorAll returns a non-empty list before accessing the first element? & maybe should also iterate through the list and add the class name to all selected elements?
| // Optional callback function when source link is clicked. | ||
| clickFn?: ClickFnType; | ||
| // Optional callback function to hide the tile when it only has single dot. | ||
| hideTileFn?: () => void; |
There was a problem hiding this comment.
nit: if I'm understanding this correctly, it looks like this callback is really a callback to handle when there is a single dot. Place page handles this by hiding the tile, but other callers of this tile could technically handle it differently. Maybe update the name of the prop and the description?
| sources: Set<string>; | ||
| handleEmbed?: () => void; | ||
| exploreJsx?: JSX.Element; | ||
| // Optional callback function when source link is clicked. |
There was a problem hiding this comment.
not just for source link right? also used for export?
| // set of full urls of sources of the data in the chart | ||
| sources: Set<string>; | ||
| handleEmbed?: () => void; | ||
| export interface ClickFnType { |
There was a problem hiding this comment.
nit: since this is an exported interface, can we make the name more informative. Maybe something about ChartFooter like ChartFooterClickFns or something of that sort.
Also add a comment describing this type?
This revealed a bug when denominator is present (note the value is much smaller due to wrong scaling"
