-
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
Update comparison metadata to new format #513
Conversation
c55dac0
to
10d69f3
Compare
96732cd
to
3a5f4a9
Compare
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 to me, i have left a comment about the indicator influencing other filter values/other plot controls, let me know what you think!
## TODO: In current plot when you change indicator, it updates | ||
## the filters. We could support this same behaviour by making | ||
## indicator a plot control which updates the filter values | ||
## including a hidden "indicator" filter which would be the value | ||
## actually used for filtering the data. But let's check what we | ||
## actually want to do. Would have to set the x-axis too, which I don't | ||
## think we can support yet. |
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 would say that this definitely sounds a little complicated, it would be nice to keep the complexity low in the effects because while it is powerful, i can see it going out of control very quickly.
my recommendation would be to change how the users have to interact with the comparison plot slightly and instead give them preset plot control with options like "Prevalence by age", "ART number by sex" and so on, so you get to preserve the pairings of the indicator with x axis and any other filter values you want to change while still being consistent with other plots
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, I'm going to make a note of this and let's discuss when we review with Jeff and Rachel on Thursday
This PR updates the comparison metadata to the new format, there are a couple of missing things here which I've left notes about. Let's have a chat about how to address these.