Survey Answer Data Chart #249
Conversation
- Break the rendering portion of the DailyDataChart out from the Data Querying part - Reuse the rendering to grab data from surveys instead. - Support multiple lines on the line graph.
- Simplify presentation by always assuming an array of dataKeys but defaulting to a single entry array. - Support multiple colors for all chart types.
|
Very cool!
|
|
Adding @Kbalon in case she has some thoughts and/or feedback. |
Yes, each line is configured individually and the data is queried for it individually, so you can combine things.
Yes, the configuration actually takes both a step identifier and an (optional) result identifier.
Not at the moment. A non numerical value will cause a failure in the graph; I can update it to just throw those values away if they come up though (which would really be the only option to proceed and graph what is valid).
I can verify with a test, but I think the way I'm parsing the data, the later answer would be the one that would show up. I agree there's not exactly a predictable expectation for this sort of case. Seeing all the points wouldn't really be reasonable for this type of graph though; i.e. when you're making a line, which point do you run the line through?
Yup, survey name, step identifier, result identifier. I'll update the PR description for clarity.
Hover, but yes - same behavior. |
|
I think an error is probably best for non-numerical values. If someone is trying to graph results, we probably want to make it obvious there is an error in the survey values. The study team should enforce validation or a picklist. Great point about showing multiple results on the same day. Let's revisit averages and such if that comes up. Thanks! |
|
I would definitely just throw the values away if it's non-numeric rather than have the graph error; makes it robust if you ever change anything in the survey definition etc...one string could crash the whole graph etc. |
src/components/container/SurveyDataChart/SurveyDataChart.stories.tsx
Outdated
Show resolved
Hide resolved
|
@chrisnowak I worry about a team that made an error and not realizing it for some time since it wouldn't stand out. However, I could imagine there being a "skip" or "prefer not to answer" option and they wouldn't want an error because of that; so probably best to dump. |
There was a problem hiding this comment.
Overall, lg and worthwhile to have.
I had a few nits and comments on unused items.
I added a NoData story to the SurveyData component, but it was erroring (code below). Could be I am misunderstanding how to set it up. Can we have Defaults, NoData, and Live versions for the SurveyData.
/*
export const NoData = {
args: {
title: "No Data",
options: {
domainMin: 0,
lineColor: ["#e41a1c", "#377eb8", "#4daf4a"],
areaColor: ["#d41a1c", "#277eb8", "#3daf4a"]
},
intervalType: "Week",
weekStartsOn: "6DaysAgo",
lines: [{ label: "Creative Self", surveyName: "FFWEL", stepIdentifier: "CreativeSelf", resultIdentifier: "CreativeSelf", stroke: "#e41a1c" },
{ label: "Coping Self", surveyName: "FFWEL", stepIdentifier: "CopingSelf", resultIdentifier: "CopingSelf", stroke: "#377eb8" },
{ label: "Social Self", surveyName: "FFWEL", stepIdentifier: "SocialSelf", resultIdentifier: "SocialSelf", stroke: "#4daf4a" }],
valueFormatter: (value: number) => Number(value.toFixed(0)).toLocaleString(),
chartType: "Area",
previewDataProvider: (start: Date, end: Date) => {
const data: SurveyAnswersPage[] = [];
return Promise.resolve(data);
}
},
render: render
}; */
Do you think this would have satisfied that use case in a really solid manner then? I guess i'm feeling when it comes to view builder where this is just gonna not work/look good in a lot of cases...DateRangeCoordinator in view builder can't be defaulted to a specific start date for instance, so stuff is gonna move on the chart...feels like we're gonna have a lot of white space at the beginning/end of the graph for a 6 month interval etc...so for your case does it feel like it would be good enough out of box In any case, if works for a 1 month / 1 week interval, that satisfies most of the use cases i've seen so addition of 6 month isn't a big deal to me, but I would probably not make it an option for DailyDataChart since it might cause a poor UX (that much data could take a long time to query in many cases, like Apple Health steps)...
DateRangeCoordinator/DateRangeContext are really designed for specific simplified intervals; I don't think anything is saying that it's appropriate for all use cases of time series data; I think 1000% there is room for stuff that doesn't sit in a date range coordinator...it's just that we have a lot of use cases existing where it makes sense to navigate on a day/week/month basis...plenty of summary use cases where we may want to graph longer-term longitudinal data and those likely require a different approach (i.e. calculating date range based on data) |
I think this would have gotten me 99% of the way there. You're correct - from within ViewBuilder I would have still been slightly stuck - but I didn't build the project in ViewBuilder. So I would have probably used the DRC but passed it an explicit start date, because it does support that, it's just not being exposed. Or, I would have not had the DRC at all and just specified the start interval and intervalType directly on the SurveyDataChart. But, if ViewBuilder supported setting the startInterval on either the SurveyDataChart with no DRC or setting it on the DRC, I would have been totally fine. |
|
Got it, makes sense. I mostly wanted to trace a path forward to view builder and make sure we were satisfying 1 anchor use case. Having 6 months as an option on SurveyAnswerChart makes sense to me; it is mostly on DateRangeCoordinator where it feels weird; since as you pointed out you may not have used it there at all and just anchored it to a specific date (which feels like the right answer to me in your case if the study is less than 6 mos long) So maybe when we add this to view builder I would suggest we:
That's my 2c though, i'll try to do a full review of this PR tomorrow |
Most of this I agree with; I do have some extra thoughts on it that are not really relevant here. Maybe worth doing a quick re-group on Slack with other folks that have been deal with dates/graphs/view builder as a whole - or maybe we just have another long discussion on a View Builder Issue instead. |
- isToday styling bolds *everything* on intraday which isn't intentional
|
@CodiumAI-Agent /review |
PR Review 🔍
Code feedback:
|
| } | ||
|
|
||
| export interface BarChartOptions { | ||
| barColor?: ColorDefinition | ColorDefinition[], |
There was a problem hiding this comment.
Would definitely NOT do parallel arrays here...we should definitely figure out another way to structure this such that colors are attached to the actual series that is being represented here; it took me a while just to figure out how coloring each survey answer would work.
Realistically if we think about how you'd set this up in view builder; you'd want to add a survey answer to the chart and specify a color specific to that answer...I would suggest that each dataKey here is an object that has a color associated with it, and your SurveyAnswerChartParameters (or whatever we name it) has a color on it
There was a problem hiding this comment.
So basically to figure out the color of the bar it would:
- Check the thresholds; if over a threshold, use the threshold color
- If the dataKey has a color specified, use the dataKey color
- If the options has a color specified, use that color
- Otherwise use
var(--mdhui-color-primary)
There was a problem hiding this comment.
I wrestled with this a bit because the parallel arrays didn't make me very happy either. I was trying to walk a balance of not having 3 different interfaces for doing things between 3 different related but different widgets (the DailyDataChart, the SurveyAnswerChart, and the TimeSeriesChart). I was trying to avoid having the DailyData and SurveyAnswer have "custom" interfaces that they each "adapted" into something for the TimeSeriesChart because that seemed more confusing. In particular I really wanted them to be able to share these base FooChartOptions classes. And in doing so, I was trying to not break the current interface of the DailyDataChart, so that any current users could keep on working, and I wouldn't have to change View Builder.
Honestly, I feel strongly that changing the dataKeys for the TimeSeriesChart into objects instead of the current string array doesn't make it clearer. I think that having two separate places where you can define the colors is harder to discover and understand as an end user, and the fact that we'd have to convey the hierarchy of where we look for and prioritize those colors is a situation I'd prefer to avoid.
Here's two suggestions:
- Move the
dataKeysproperty off of theTimeSeriesChartPropsand into eachFooChartOptions.
export interface LineChartOptions {
lineColor?: ColorDefinition | ColorDefinition[],
dataKeys?: undefined | string[],
connectNulls?: boolean,
domainMin?: number | "Auto"
}
export interface BarChartOptions {
barColor?: ColorDefinition | ColorDefinition[],
dataKeys?: undefined | string[],
thresholds?: BarChartThreshold[]
}
This at least colocates the parallel arrays so they're next to each other.
- Similar solution, but more like your suggestion, again removing the
dataKeysproperty fromTimeSeriesChart:
export interface LineChartOptions {
lineColor?: ColorDefinition,
seriesDefinition? : undefined | { dataKey: string, lineColor: ColorDefinition }[]
connectNulls?: boolean,
domainMin?: number | "Auto"
}
export interface BarChartOptions {
barColor?: ColorDefinition,
seriesDefinition? : undefined | { dataKey: string, barColor: ColorDefinition }[]
thresholds?: BarChartThreshold[]
}
This has the issue of having to "prioritize" things, but at least in this case the properties are much closer together and maybe its more obvious?
src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| export interface LineChartOptions { | ||
| lineColor?: ColorDefinition | ColorDefinition[], | ||
| connectNulls?: boolean, |
There was a problem hiding this comment.
Hmm yeah, for survey answer data I can see this make sense since you might have large gaps in between
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
- Model Month behavior; by default go 6 months back from the current month, all the way back to the 1st. - Fix tick mark generation on TimeSeriesChart for multiple possibilities of how the 6 month span starts, to always get the ticks in predictable spots. - Add stories to demonstrate this behavior.
…eriesChart - This was preventing the Loading indicator from ever being presented.
Can re-use because DailyDataChart is prevented from calling with 6Month parameter by other type checking.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| thresholds?: BarChartThreshold[] | ||
| } | ||
|
|
||
| export interface BarChartThreshold { |
There was a problem hiding this comment.
Can we simplify this name to Threshold?
There was a problem hiding this comment.
I would prefer not to. I think having such a generic name isn't helpful to have as an export when it's use is pretty narrow to its usage in this class.
There was a problem hiding this comment.
I'm indifferent to what it is named if Threshold is too generic.
ChartThresholds is another option.
Given a threshold lines are shown in both bar and line, it is confusing for the name to be bar chart specific.
Overview
This PR adds a
SurveyDataChartContainer component. This chart will graph responses to a Survey over time (configured by specifying the Survey Name, Step Identifier, and optionally the Result Identifier). The presentation here was driven by a component I originally created for a customer where they have multiple surveys that calculate multiple scores per survey, and they want to see those scores side by side over time. As a specific example, the project was giving repeated instances of the Five Factor Wellness Inventory; this survey generates a total score and five second order scores, and the aim was to show all of those scores over time. The storybook entry I created is a simplified version of this:The bigger swing I took here was to attempt to leverage the rendering code already established in the
DailyDataChartby refactoring it into a more genericDataChartcomponent that is used by both theDailyDataChartand theSurveyDataChart. This is a pattern that I think we should really be following more often; separating the responsibilities of retrieving data and presenting data. I've run into several cases in using the MDHUI library where I couldn't re-use one of the components because I needed to make the slightest tweak to the set of data being presented and couldn't because we're not separating these concerns (i.e. theSurveyTaskListgets a specific set of Tasks, rather than being a presentational component which renders a list of Tasks it's given, and we also provide anOpenSurveyTaskListwhich gets the data). So I took the plunge in this PR to try and start introducing and encouraging this pattern.Behaviorally, the only change I ended up making to the rendering code was the addition of being able to display multiple sets of data on one graph (something we'd had in the
DeviceDataMonthChartbut then did not support in theDailyDataChart). I did this by having an optionaldataKeysparameter for theDataChart. If you don't pass any, it assumes a basic default key ofvaluewhich is how theDailyDataChartformats its basic data. So the Survey Chart can support lines, as shown above, as well as area and bar presentations (though you can get some truly awful color combinations of areas):I also made it so that the "parent" container is the one who specifies how the hover detail works. This was mostly for handling rather different formatting if there's only one line versus multiple.
Backwards compatibility for the
DailyDataChartshould be unchanged; it's external interface was unmodified. All stories for it should still work, as verification.Security
REMINDER: All file contents are public.
Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.
Checklist
Testing
Documentation
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:
Summary by CodeRabbit
New Features
SurveyDataChartfor rendering various types of survey data charts.DateRangeCoordinatorandDateRangeNavigator.TimeSeriesChartwith customizable options for line, bar, and area charts.Improvements
DailyDataChartto use the newTimeSeriesChartcomponent for better consistency and customization.Bug Fixes
SurveyDataChartfor better accuracy and user experience.