-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NM-95] Update naomi population pyramid to not filter aggregated proportions on selected area #1043
Conversation
} | ||
|
||
export type PlotData = CalibrateDataResponse["data"] | InputTimeSeriesData | CalibratePlotData | ComparisonPlotData | InputComparisonData | PopulationPyramidData; | ||
export type SquarePlotData = Exclude<PlotData, PopulationPyramidData>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is quite ugly... but not sure if there a better way to do this? Thought about committing the national data to some other place in state but that seemed quite ugly too. Or doing the national filtering later, but that it a big gross... very happy to hear if you have any good ideas, I am a little stumped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is a bit awkward but I think it makes most sense to do the aggregating of national data and all other data in the same place as we have currently. Only other thing I can think is, as you said, making two state commits in the getPopulationFilteredData function and committing the national level data to a different place in state. The two data sets are so closely tied together though and are really only relevant to the plot so I'm inclined to say just go with the method you have of coupling the two data sets together in an object. We could rename the lowest "data" field to something like "subPlotData" to avoid the data.data repetition but not entirely necessary
@@ -45,7 +45,7 @@ export default defineComponent({ | |||
}, | |||
setup(props) { | |||
const store = useStore<RootState>(); | |||
const plotData = computed<PlotData>(() => store.state.plotData[props.plotName]); | |||
const plotData = computed<SquarePlotData>(() => store.state.plotData[props.plotName] as SquarePlotData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to change the type for the data prop in TableReshapeData.vue to match with the new SquarePlotData type. Getting a TS compiler error in this file on line 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that, weirdly I am not getting that. Different typescript versions maybe? But I have changed this type instead to TableData
and made it specific to those types we declare for the prop in TableReshapeData. I'm hoping that might work? And I think reads a bit nicer than square plot data...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, no compiler errors now. Agreed on the name change as well
Description
See https://teams.microsoft.com/l/message/19:83f74f91587b4848a35c45bca0c3a2b1@thread.tacv2/1733081977154?tenantId=2b897507-ee8c-4575-830b-4f8267c3d307&groupId=e24ac85e-dd9e-4505-8b9e-047b3a5fa6f0&parentMessageId=1733081977154&teamName=HIV%20Inference%20Group%20-%20WP&channelName=Naomi&createdTime=1733081977154 for some context, the national boundaries are being calculated after filtering. So if they are filtering the area level down to a few districts it won't be the correct boundary.
This fixes that by calculating aggregated national data removing the area filter. We don't want to apply the area filter. I'd like to get this in as a quick fix, but I think there are some opportunities for tidying up this approach.
Type of version change
Patches
Checklist