[NM-62] Add population pyramid plot #1041
Conversation
|
/update-snapshots |
There was a problem hiding this comment.
From a first look, this looks excellent. Couple of small comments I've left you but this is looking really good. Components are impressively tidy for the amount of logic you had to get in there.
Re formatting the tooltips to show +ve I think I can handle that in metadata. We use numeral js http://numeraljs.com/#format formats. This is what formatOutput function uses under the hood. So I think I can force those to show +ve by wrapping the current format in brackets.
I also spotted an issue with a few countries, I tried with Kenya, Angola and DRC. If you import Kenya and select any area level higher than the lowest. I get
Uncaught (in promise) TypeError: fullParentMap[area_id] is undefined
aggregatePopulationIndicators filter.ts:305
aggregatePopulationIndicators filter.ts:303
_callee7$ filter.ts:325
Babel 6
| if (isProportion.value) { | ||
| return (value * 100).toFixed(0) + "%"; | ||
| } else { | ||
| return value.toLocaleString(); | ||
| } |
There was a problem hiding this comment.
For these, can you use formatOutput helper function. Should be able to call it with metadata so
formatOutput(value, plotTypeMetadata.format, plotTypeMetadata.scale, plotTypeMetadata.accuracy)
There was a problem hiding this comment.
Thanks for this, it's exhibiting the same issue as the tooltip now with negative values on the left side of x-axis, but once we add some kind of absolute value fix it should resolve in both places.
|
|
||
| const subplotsConfig = { | ||
| columns: 3, | ||
| rows: 4, |
There was a problem hiding this comment.
I wonder about trying 3 rows, and increasing the height of each? It does look a little fiddly to me to make sure your cursor is on the correct bar for the tooltip and a bit of extra height might help?
There was a problem hiding this comment.
Switched to 3x3 grid, increased the aspect ratio slightly which should increase the height of each chart while still keeping them responsive. I agree that this looks better, seems to help with the tooltip too though it's still a little touchy
| const pageNumber = ref<number>(0); | ||
|
|
||
| watch( | ||
| () => store.state.plotSelections["population"], | ||
| () => (pageNumber.value = 0) | ||
| ); |
There was a problem hiding this comment.
Might be nice to move this page number logic into a composable. https://vuejs.org/guide/reusability/composables.
I think that would be appropriate, @M-Kusumgar probably knows better if I am talking nonsense or not.
Not a blocker to getting this merged though.
| outlineData.forEach((dataset: any, datasetIndex: number) => { | ||
| if (datasetIndex < 2) return; | ||
| const meta = chart.getDatasetMeta(datasetIndex); | ||
|
|
||
| const points: any[] = []; | ||
|
|
||
| meta.data.forEach((bar: any, index: number) => { | ||
| const { x, y, height, width } = bar; | ||
|
|
||
| // Calculate the coordinates for the outer corners of the bar | ||
| const topY = y - height / 2 - 0.5; | ||
| const bottomY = y + height / 2 + 0.5; | ||
|
|
||
| // Connect top line horizontally to y-axis instead of connecting back to the starting point | ||
| if (index === 0) { | ||
| points.push({ | ||
| y: topY, | ||
| x: datasetIndex === 2 ? x - width : x + width, | ||
| }); | ||
| } | ||
|
|
||
| points.push({ x: x, y: topY }); | ||
| points.push({ x: x, y: bottomY }); | ||
|
|
||
| }); | ||
|
|
||
| ctx.beginPath(); | ||
| ctx.moveTo(points[0].x, points[0].y); | ||
| for (let i = 1; i < points.length; i++) { | ||
| ctx.lineTo(points[i].x, points[i].y); | ||
| } | ||
| ctx.stroke(); | ||
| }); | ||
|
|
||
| ctx.restore(); | ||
| }, |
There was a problem hiding this comment.
This must have been well fiddly to work out!
| await page.getByRole('button', { name: 'Population' }).click(); | ||
| await page.locator('a').filter({ hasText: 'Population ratio' }).click(); | ||
|
|
||
| // // Table has been updated |
There was a problem hiding this comment.
Can you add a screenshot test with an aggregated area level?
|
The errors you were getting with Kenya, DRC etc. should be fixed now. The way the aggregation logic was written it was making some incorrect assumptions about population data being uploaded only at the most disaggregated level. Modified the filtering logic a bit, and also added a check when aggregating to see if an uploaded indicator already exists. If it does exist, it will now be used directly, and if not it will be built from aggregated child indicators as it was doing previously. |
|
/update-snapshots |
1 similar comment
|
/update-snapshots |
|
/update-snapshots |
1 similar comment
|
/update-snapshots |
…plot selection utils tests
…ling, add pagination)
… multiple area levels
… is more disaggregated than uploaded data
…ts for bubble plot
|
/update-snapshots |
Description
Adds population plot to review inputs step. One component for individual charts and another for the full grid with pagination. Logic is implemented to aggregate population data where necessary (i.e. if user hasn't uploaded data at a particular area level). This is based on the assumption that if a user has uploaded population data at a particular area level, it is complete (this validation appears to be handled already on upload of the population file).
Stepped outline showing national population and corresponding note will appear when the population ratio plot type is selected. We still want to change the label here from "Population ratio" to "Population proportion" at some point. Additionally, we may want to remove area level options that are more disaggregated than the users uploaded data (i.e. disallow filtering by District if the user has only uploaded Region data). Currently there is a guard clause in place which will force the no data message to appear if a user runs into this situation.
Type of version change
Minor
Checklist