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
}; */
| } | ||
|
|
||
| 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?
There was a problem hiding this comment.
Hmm...yeah neither option here feels right to me.
1 as you point out is still parallel arrays
2 is really burying a key required top-level property into sub-sub object so our chart would be like <TimeSeriesChart options={{seriesDefinition:{dataKey:"Steps", barColor:"blue"}}}/>
which feels rough.
I would certainly not break the DailyDataChart interface/props, but I think it'd be ok to have a distinct DailyDataChartLineOptions, DailyDataChartBarChartOptions, etc. interfaces so we have freedom here...just changing types is not enough of a breaking change that I would feel like we'd need to cut a new major. Or we can name these new options "TimeSeriesChartOptions"
So I think we do that^ and keep those types the same. And then we can split our "options" from "series" such that we have:
export interface TimeSeriesChartProps {
title?: string
intervalType?: "Week" | "Month" | "6Month" | "Day",
intervalStart: Date,
data: any[] | undefined,
expectedDataInterval?: Duration,
series: ChartSeries[]
chartHasData: boolean,
tooltip: ({ active, payload, label }: any) => React.JSX.Element | null,
chartType: "Line" | "Bar" | "Area"
options?: TimeSeriesLineChartOptions | TimeSeriesBarChartOptions
innerRef?: React.Ref<HTMLDivElement>
}
export interface ChartSeries {
dataKey: string
color: ColorDefinition
}
export interface AreaChartSeries extends ChartSeries {
areaColor: ColorDefinition // not sure how many use cases for multi-series area charts we have (or area charts in general), so could go either way on this
}
export interface TimeSeriesLineChartOptions {
connectNulls?: boolean,
domainMin?: number | "Auto"
}
export interface TimeSeriesBarChartOptions {
thresholds?: BarChartThreshold[]
}
export interface TimeSeriesBarChartThreshold {
value: number
referenceLineColor?: ColorDefinition
overThresholdBarColor?: ColorDefinition
}
Something like that? And now that I look at that ^ we could consider dropping "options" entirely. domainMin probably makes sense for all charts I think...not sure why it was just added to lines. And sounds like we're adding thresholds to line charts as well...not sure if that is coming here or in @reneefromhold 's PR. The only kinda oddball one feels like "connectNulls" which i'd be fine just putting on TimeSeriesChartProps even if it doesn't make sense for bar/area charts right now.
There was a problem hiding this comment.
I've implemented this with the slight adjustment that instead of calling it TimeSeriesLineChartOptions I called them MultipleSeriesLineChartOptions because then the SurveyAnswerChart and TimeSeriesChart both share a similar interface because they both support showing multiple series of data. The DailyDataChart is the only one that converts from its own property styles to the interface that TimeSeriesChart wants. I've adjusted all the stories as necessary.
I'd talked to Renee and asked if she would add thresholds to line charts in her PR as this PR was large enough as it was, and adding it in her PR where it would be used the first time seemed appropriate.
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.
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
Avoids parallel arrays for keys and colors. Maintain backwards interface to DailyDataChart for compatibility purposes by mapping single chart series properties to multiple series properties.
WalkthroughThe updates primarily enhance charting capabilities and interval handling within various components. Significant changes include the introduction of new chart configurations, support for a "6Month" interval type, and the addition of new helper functions for interval calculations and random data generation. The refactoring of data processing and chart rendering functions, along with the introduction of the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review as they are similar to previous changes (1)
Additional context usedBiome
Additional comments not posted (4)
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range and nitpick comments (6)
src/components/presentational/DateRangeNavigator/DateRangeNavigator.stories.tsx (2)
Line range hint
2-3: Mark imports used only as types withtype.- import React from "react"; - import { ComponentStory, ComponentMeta } from "@storybook/react"; + import type React from "react"; + import type { ComponentStory, ComponentMeta } from "@storybook/react";Also applies to: 4-5
Line range hint
19-19: Useletorconstinstead ofvarfor variable declarations.- var currentDate = new Date(); + const currentDate = new Date();src/helpers/get-interval-start.ts (1)
Line range hint
5-11: Simplify the computed expressions and refactor unnecessaryelseclauses.- let dayOfWeekLookup = { + const dayOfWeekLookup = { "Sunday": 0, "Monday": 1, "Tuesday": 2, "Wednesday": 3, "Thursday": 4, "Friday": 5, "Saturday": 6 } - if (weekStartsOn === "6DaysAgo" || weekStartsOn === "7DaysAgo") { + if (["6DaysAgo", "7DaysAgo"].includes(weekStartsOn)) { let startDate = add(today, { days: weekStartsOn === "6DaysAgo" ? -6 : -7 }); return startDate; - } else { + } let dayOfWeek = dayOfWeekLookup[weekStartsOn]; let dayOfWeekToday = today.getDay(); let daysToSubtract = dayOfWeekToday - dayOfWeek; if (daysToSubtract < 0) { daysToSubtract = 7 + daysToSubtract; } let startDate = add(today, { days: -daysToSubtract }); return startDate; - }Also applies to: 24-33
src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.tsx (2)
Line range hint
1-1: Mark imports used only as types withtype.- import React, { useEffect, useState } from "react"; - import { createContext } from "react"; + import type React, { useEffect, useState } from "react"; + import type { createContext } from "react";Also applies to: 3-4
Line range hint
38-38: Correct theuseEffectdependencies to includeinitialIntervalStart.- }, [props.intervalType, props.weekStartsOn]); + }, [props.intervalType, props.weekStartsOn, initialIntervalStart]);src/components/presentational/DateRangeNavigator/DateRangeNavigator.tsx (1)
Line range hint
31-39: Convert function expressions to arrow functions for consistency and improved readability.- var nextInterval = function () { + const nextInterval = () => { - var previousInterval = function () { + const previousInterval = () => {Arrow functions provide a cleaner and more concise syntax, especially when the function does not require its own
thiscontext.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (22)
- src/components/container/DailyDataChart/DailyDataChart.stories.tsx (2 hunks)
- src/components/container/DailyDataChart/DailyDataChart.tsx (3 hunks)
- src/components/container/DailyDataChart/index.ts (1 hunks)
- src/components/container/SurveyAnswerChart/SurveyAnswerChart.stories.tsx (1 hunks)
- src/components/container/SurveyAnswerChart/SurveyAnswerChart.tsx (1 hunks)
- src/components/container/SurveyAnswerChart/index.ts (1 hunks)
- src/components/container/index.ts (2 hunks)
- src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.stories.tsx (3 hunks)
- src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.tsx (2 hunks)
- src/components/presentational/DateRangeNavigator/DateRangeNavigator.stories.tsx (1 hunks)
- src/components/presentational/DateRangeNavigator/DateRangeNavigator.tsx (2 hunks)
- src/components/presentational/TimeSeriesChart/TimeSeriesChart.css (3 hunks)
- src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx (1 hunks)
- src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx (1 hunks)
- src/components/presentational/TimeSeriesChart/index.ts (1 hunks)
- src/components/presentational/index.ts (1 hunks)
- src/helpers/chartOptions.ts (1 hunks)
- src/helpers/date-helpers.ts (1 hunks)
- src/helpers/get-interval-start.ts (2 hunks)
- src/helpers/index.ts (1 hunks)
- src/helpers/predictableRandomNumber.ts (1 hunks)
- src/helpers/query-daily-data.tsx (2 hunks)
Files not reviewed due to errors (1)
- src/components/container/SurveyAnswerChart/SurveyAnswerChart.stories.tsx (no review received)
Files skipped from review due to trivial changes (4)
- src/components/container/DailyDataChart/index.ts
- src/components/container/SurveyAnswerChart/index.ts
- src/components/presentational/TimeSeriesChart/index.ts
- src/helpers/index.ts
Additional context used
Biome
src/helpers/predictableRandomNumber.ts
[error] 8-8: Use Number.parseInt instead of the equivalent global.
src/helpers/chartOptions.ts
[error] 1-1: All these imports are only used as types.
src/components/presentational/DateRangeNavigator/DateRangeNavigator.stories.tsx
[error] 2-3: All these imports are only used as types.
[error] 4-5: Some named imports are only used as types.
[error] 19-19: Use let or const instead of var.
src/helpers/get-interval-start.ts
[error] 5-5: The computed expression can be simplified without the use of a string literal.
[error] 6-6: The computed expression can be simplified without the use of a string literal.
[error] 7-7: The computed expression can be simplified without the use of a string literal.
[error] 8-8: The computed expression can be simplified without the use of a string literal.
[error] 9-9: The computed expression can be simplified without the use of a string literal.
[error] 10-10: The computed expression can be simplified without the use of a string literal.
[error] 11-11: The computed expression can be simplified without the use of a string literal.
[error] 24-33: This else clause can be omitted because previous branches break early.
[error] 3-4: This let declares a variable that is only assigned once.
[error] 18-18: Reassigning a function parameter is confusing.
[error] 20-20: This let declares a variable that is only assigned once.
[error] 22-22: This let declares a variable that is only assigned once.
[error] 25-25: This let declares a variable that is only assigned once.
[error] 26-26: This let declares a variable that is only assigned once.
[error] 31-31: This let declares a variable that is only assigned once.
[error] 37-37: This let declares a variable that is only assigned once.
[error] 38-38: This let declares a variable that is only assigned once.
src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.tsx
[error] 1-1: The default import is only used as a type.
[error] 3-4: Some named imports are only used as types.
[error] 38-38: This hook does not specify all of its dependencies: initialIntervalStart
[error] 38-38: This hook specifies more dependencies than necessary: props.weekStartsOn
src/helpers/date-helpers.ts
[error] 12-12: This var should be declared at the root of the enclosing function.
[error] 13-13: This var should be declared at the root of the enclosing function.
[error] 50-50: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 50-50: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 53-53: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 53-53: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 56-67: This else clause can be omitted because previous branches break early.
[error] 56-56: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 56-56: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 59-67: This else clause can be omitted because previous branches break early.
[error] 59-59: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 59-59: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 59-59: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 62-67: This else clause can be omitted because previous branches break early.
[error] 62-62: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 11-11: Use let or const instead of var.
[error] 12-12: Use let or const instead of var.
[error] 13-13: Use let or const instead of var.
[error] 20-20: Use let or const instead of var.
[error] 21-21: Use let or const instead of var.
src/components/presentational/DateRangeNavigator/DateRangeNavigator.tsx
[error] 26-26: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 27-27: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 28-28: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 31-34: This function expression can be turned into an arrow function.
[error] 36-39: This function expression can be turned into an arrow function.
[error] 49-49: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 4-5: All these imports are only used as types.
[error] 26-29: Use let or const instead of var.
[error] 31-34: Use let or const instead of var.
[error] 32-32: Use let or const instead of var.
[error] 36-39: Use let or const instead of var.
[error] 37-37: Use let or const instead of var.
[error] 41-41: Use let or const instead of var.
[error] 42-42: Use let or const instead of var.
[error] 43-43: Use let or const instead of var.
[error] 48-48: This let declares a variable that is only assigned once.
src/helpers/query-daily-data.tsx
[error] 37-37: Template literals are preferred over string concatenation.
[error] 46-46: Template literals are preferred over string concatenation.
[error] 50-50: This var should be declared at the root of the enclosing function.
[error] 47-57: This function expression can be turned into an arrow function.
[error] 66-66: This var should be declared at the root of the enclosing function.
[error] 67-67: This var should be declared at the root of the enclosing function.
[error] 67-67: Template literals are preferred over string concatenation.
[error] 82-82: Forbidden non-null assertion.
[error] 85-87: Prefer for...of instead of forEach.
[error] 85-87: This function expression can be turned into an arrow function.
[error] 3-4: All these imports are only used as types.
[error] 15-15: This let declares a variable that is only assigned once.
[error] 36-36: Use let or const instead of var.
[error] 54-54: Reassigning a function parameter is confusing.
[error] 45-45: Use let or const instead of var.
[error] 48-48: Use let or const instead of var.
[error] 50-50: Use let or const instead of var.
[error] 69-69: Reassigning a function parameter is confusing.
[error] 61-61: Use let or const instead of var.
[error] 62-62: This let declares a variable that is only assigned once.
src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.stories.tsx
[error] 30-30: The computed expression can be simplified without the use of a string literal.
[error] 31-31: The computed expression can be simplified without the use of a string literal.
[error] 32-32: The computed expression can be simplified without the use of a string literal.
[error] 33-33: The computed expression can be simplified without the use of a string literal.
[error] 34-34: The computed expression can be simplified without the use of a string literal.
[error] 35-35: The computed expression can be simplified without the use of a string literal.
[error] 36-36: The computed expression can be simplified without the use of a string literal.
[error] 37-37: The computed expression can be simplified without the use of a string literal.
[error] 38-38: The computed expression can be simplified without the use of a string literal.
[error] 39-39: The computed expression can be simplified without the use of a string literal.
[error] 42-42: The computed expression can be simplified without the use of a string literal.
[error] 73-73: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
[error] 3-4: Some named imports are only used as types.
[error] 9-10: All these imports are only used as types.
[error] 12-13: This let declares a variable that is only assigned once.
[error] 14-15: This let declares a variable that is only assigned once.
[error] 45-45: Use let or const instead of var.
[error] 51-51: This let declares a variable that is only assigned once.
src/components/container/DailyDataChart/DailyDataChart.tsx
[error] 49-49: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 50-50: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 73-75: This function expression can be turned into an arrow function.
[error] 97-97: Unexpected any. Specify a different type.
[error] 98-98: This type annotation is trivially inferred from its initialization.
[error] 100-119: Prefer for...of instead of forEach.
[error] 102-102: Unexpected any. Specify a different type.
[error] 105-105: Forbidden non-null assertion.
[error] 107-107: Forbidden non-null assertion.
[error] 124-124: Unexpected any. Specify a different type.
[error] 126-126: This var should be declared at the root of the enclosing function.
[error] 157-179: This else clause can be omitted because previous branches break early.
[error] 168-179: This else clause can be omitted because previous branches break early.
[error] 1-1: The default import is only used as a type.
[error] 2-3: Some named imports are only used as types.
[error] 6-7: Some named imports are only used as types.
[error] 9-10: All these imports are only used as types.
[error] 28-28: This let declares some variables that are only assigned once.
[error] 29-29: This let declares some variables that are only assigned once.
[error] 49-49: This let declares a variable that is only assigned once.
src/components/container/DailyDataChart/DailyDataChart.stories.tsx
[error] 141-141: Template literals are preferred over string concatenation.
[error] 142-142: Template literals are preferred over string concatenation.
[error] 162-162: Template literals are preferred over string concatenation.
[error] 163-163: Template literals are preferred over string concatenation.
[error] 183-183: Template literals are preferred over string concatenation.
[error] 184-184: Template literals are preferred over string concatenation.
[error] 203-203: Template literals are preferred over string concatenation.
[error] 204-204: Template literals are preferred over string concatenation.
[error] 223-223: Template literals are preferred over string concatenation.
[error] 224-224: Template literals are preferred over string concatenation.
[error] 243-243: Template literals are preferred over string concatenation.
[error] 244-244: Template literals are preferred over string concatenation.
[error] 1-2: Some named imports are only used as types.
[error] 3-4: Some named imports are only used as types.
[error] 7-8: This let declares a variable that is only assigned once.
[error] 39-39: This let declares a variable that is only assigned once.
[error] 43-43: This let declares a variable that is only assigned once.
[error] 140-140: Use let or const instead of var.
[error] 141-141: Use let or const instead of var.
[error] 161-161: Use let or const instead of var.
src/components/container/SurveyAnswerChart/SurveyAnswerChart.tsx
[error] 51-51: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 72-74: This function expression can be turned into an arrow function.
[error] 83-83: This var should be declared at the root of the enclosing function.
[error] 94-94: Unexpected any. Specify a different type.
[error] 95-95: This type annotation is trivially inferred from its initialization.
[error] 98-113: Prefer for...of instead of forEach.
[error] 100-112: Prefer for...of instead of forEach.
[error] 100-100: Forbidden non-null assertion.
[error] 103-103: Forbidden non-null assertion.
[error] 108-108: Forbidden non-null assertion.
[error] 117-117: Unexpected any. Specify a different type.
[error] 118-118: Unexpected any. Specify a different type.
[error] 120-120: Unexpected any. Specify a different type.
[error] 121-121: Avoid redundant double-negation.
[error] 135-135: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 149-149: Unexpected any. Specify a different type.
[error] 172-172: Unexpected any. Specify a different type.
[error] 1-1: The default import is only used as a type.
[error] 3-4: All these imports are only used as types.
[error] 4-5: Some named imports are only used as types.
src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx
[error] 13-13: Unexpected any. Specify a different type.
[error] 17-17: Unexpected any. Specify a different type.
[error] 18-18: Unexpected any. Specify a different type.
[error] 57-57: Template literals are preferred over string concatenation.
[error] 58-58: Template literals are preferred over string concatenation.
[error] 59-59: Template literals are preferred over string concatenation.
[error] 67-67: Unexpected any. Specify a different type.
[error] 85-85: Unexpected any. Specify a different type.
[error] 88-88: Unexpected any. Specify a different type.
[error] 85-97: This function expression can be turned into an arrow function.
[error] 1-1: All these imports are only used as types.
[error] 3-4: Some named imports are only used as types.
[error] 12-13: This let declares a variable that is only assigned once.
[error] 24-24: Use let or const instead of var.
[error] 37-37: Use let or const instead of var.
[error] 52-52: Use let or const instead of var.
[error] 67-67: Use let or const instead of var.
[error] 70-70: This let declares a variable that is only assigned once.
[error] 84-97: Use let or const instead of var.
src/components/container/SurveyAnswerChart/SurveyAnswerChart.stories.tsx
[error] 15-15: The computed expression can be simplified without the use of a string literal.
[error] 16-16: The computed expression can be simplified without the use of a string literal.
[error] 17-17: The computed expression can be simplified without the use of a string literal.
[error] 18-18: The computed expression can be simplified without the use of a string literal.
[error] 19-19: The computed expression can be simplified without the use of a string literal.
[error] 20-20: The computed expression can be simplified without the use of a string literal.
[error] 21-21: The computed expression can be simplified without the use of a string literal.
[error] 22-22: The computed expression can be simplified without the use of a string literal.
[error] 23-23: The computed expression can be simplified without the use of a string literal.
[error] 24-24: The computed expression can be simplified without the use of a string literal.
[error] 27-27: The computed expression can be simplified without the use of a string literal.
[error] 43-43: Unexpected any. Specify a different type.
[error] 43-43: Unexpected any. Specify a different type.
[error] 2-3: Some named imports are only used as types.
[error] 3-4: All these imports are only used as types.
[error] 9-10: This let declares a variable that is only assigned once.
[error] 13-13: Use let or const instead of var.
[error] 32-32: This let declares a variable that is only assigned once.
[error] 33-33: This let declares a variable that is only assigned once.
[error] 34-34: This let declares a variable that is only assigned once.
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
[error] 18-18: Unexpected any. Specify a different type.
[error] 22-22: Unexpected any. Specify a different type.
[error] 32-32: Unexpected any. Specify a different type.
[error] 35-35: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 37-63: This else clause can be omitted because previous branches break early.
[error] 37-37: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 44-63: This else clause can be omitted because previous branches break early.
[error] 44-44: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 45-45: This type annotation is trivially inferred from its initialization.
[error] 47-47: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 57-63: This else clause can be omitted because previous branches break early.
[error] 57-57: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 76-106: This else clause can be omitted because previous branches break early.
[error] 77-77: This var should be declared at the root of the enclosing function.
[error] 78-78: This var should be declared at the root of the enclosing function.
[error] 81-106: This else clause can be omitted because previous branches break early.
[error] 82-82: This var should be declared at the root of the enclosing function.
[error] 83-83: This var should be declared at the root of the enclosing function.
[error] 87-87: This var should be declared at the root of the enclosing function.
[error] 93-93: This var should be declared at the root of the enclosing function.
Additional comments not posted (2)
src/components/presentational/TimeSeriesChart/TimeSeriesChart.css (1)
Line range hint
1-70: CSS changes are consistent and well-structured.src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx (1)
77-78: Declare variables at the root of their enclosing function.
[REFACTOR_SUGGESTion]
Variables likemonthLength,numberOfTicks,ticks,currentTick, andfirstTickshould be declared at the top of their enclosing function to improve readability and maintainability.Also applies to: 82-83, 87-93
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/container/SurveyAnswerChart/SurveyAnswerChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/TimeSeriesChart/TimeSeriesChart.tsx
Outdated
Show resolved
Hide resolved
src/components/presentational/DateRangeCoordinator/DateRangeCoordinator.stories.tsx
Show resolved
Hide resolved
src/components/container/SurveyAnswerChart/SurveyAnswerChart.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (1)
src/components/presentational/DateRangeNavigator/DateRangeNavigator.tsx (1)
Line range hint
49-49: Replace==with===for strict equality checks to avoid type coercion issues.- if (props.variant == "rounded") { + if (props.variant === "rounded") {
|
|
||
| function processPages(pages: SurveyAnswer[][]) { | ||
| var newDailyData: { [key: string]: SurveyAnswer[] } = {}; | ||
| for (var i = 0; i < props.series.length; i++) { |
There was a problem hiding this comment.
Replace var with const for block-scoped variable declarations and avoid using any by specifying more precise types.
- var newDailyData: { [key: string]: SurveyAnswer[] } = {};
+ const newDailyData: { [key: string]: SurveyAnswer[] } = {};
- var data: any[] | undefined = undefined;
+ const data: { timestamp: number, [key: string]: number }[] | undefined = undefined;
- var tooltip = ({ active, payload, _label }: any): React.JSX.Element | null => {
+ const tooltip = ({ active, payload, _label }: { active: boolean, payload: any[], _label: string }): React.JSX.Element | null => {Also applies to: 94-94, 117-117, 118-118, 120-120, 121-121, 149-149, 172-172
Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 83-83: This var should be declared at the root of the enclosing function. (lint/correctness/noInnerDeclarations)
The var is accessible in the whole body of the enclosing function.
To avoid confusion, it should be declared at the root of the enclosing function.
| let intervalEnd = intervalType === "Week" ? add(intervalStart, { days: 7 }) | ||
| : intervalType === "Month" ? add(intervalStart, { months: 1 }) | ||
| : intervalType === "6Month" ? add(intervalStart, { months: 6 }) : | ||
| intervalStart; | ||
| const loadCurrentInterval = () => { | ||
| setCurrentData(null); | ||
| if (props.previewDataProvider) { | ||
| props.previewDataProvider(intervalStart, intervalEnd) | ||
| .then((data) => { | ||
| setCurrentData(processPages(data)); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| var dataRequests = props.series.map(l => queryAllSurveyAnswers({ | ||
| surveyName: l.surveyName, | ||
| stepIdentifier: l.stepIdentifier, | ||
| resultIdentifier: l.resultIdentifier, | ||
| after: intervalStart.toISOString(), | ||
| before: intervalEnd.toISOString() | ||
| })); | ||
| Promise.all(dataRequests).then((data) => { | ||
| setCurrentData(processPages(data)); | ||
| }) |
There was a problem hiding this comment.
Ensure robust error handling in asynchronous data fetching to prevent unhandled promise rejections.
+ .catch(error => {
+ console.error("Failed to load data:", error);
+ setCurrentData({});
+ })Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let intervalEnd = intervalType === "Week" ? add(intervalStart, { days: 7 }) | |
| : intervalType === "Month" ? add(intervalStart, { months: 1 }) | |
| : intervalType === "6Month" ? add(intervalStart, { months: 6 }) : | |
| intervalStart; | |
| const loadCurrentInterval = () => { | |
| setCurrentData(null); | |
| if (props.previewDataProvider) { | |
| props.previewDataProvider(intervalStart, intervalEnd) | |
| .then((data) => { | |
| setCurrentData(processPages(data)); | |
| }); | |
| return; | |
| } | |
| var dataRequests = props.series.map(l => queryAllSurveyAnswers({ | |
| surveyName: l.surveyName, | |
| stepIdentifier: l.stepIdentifier, | |
| resultIdentifier: l.resultIdentifier, | |
| after: intervalStart.toISOString(), | |
| before: intervalEnd.toISOString() | |
| })); | |
| Promise.all(dataRequests).then((data) => { | |
| setCurrentData(processPages(data)); | |
| }) | |
| let intervalEnd = intervalType === "Week" ? add(intervalStart, { days: 7 }) | |
| : intervalType === "Month" ? add(intervalStart, { months: 1 }) | |
| : intervalType === "6Month" ? add(intervalStart, { months: 6 }) : | |
| intervalStart; | |
| const loadCurrentInterval = () => { | |
| setCurrentData(null); | |
| if (props.previewDataProvider) { | |
| props.previewDataProvider(intervalStart, intervalEnd) | |
| .then((data) => { | |
| setCurrentData(processPages(data)); | |
| }) | |
| .catch(error => { | |
| console.error("Failed to load data:", error); | |
| setCurrentData({}); | |
| }); | |
| return; | |
| } | |
| var dataRequests = props.series.map(l => queryAllSurveyAnswers({ | |
| surveyName: l.surveyName, | |
| stepIdentifier: l.stepIdentifier, | |
| resultIdentifier: l.resultIdentifier, | |
| after: intervalStart.toISOString(), | |
| before: intervalEnd.toISOString() | |
| })); | |
| Promise.all(dataRequests).then((data) => { | |
| setCurrentData(processPages(data)); | |
| }) | |
| .catch(error => { | |
| console.error("Failed to load data:", error); | |
| setCurrentData({}); | |
| }); |
| return responses; | ||
| } | ||
|
|
||
| var tooltip = ({ active, payload, _label }: any): React.JSX.Element | null => { |
There was a problem hiding this comment.
Replace any with more specific types to enhance type safety in the tooltip function.
- var tooltip = ({ active, payload, _label }: any): React.JSX.Element | null => {
+ const tooltip = ({ active, payload, _label }: { active: boolean, payload: { dataKey: string, value: number }[], _label: string }): React.JSX.Element | null => {Also applies to: 84-84
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| var tooltip = ({ active, payload, _label }: any): React.JSX.Element | null => { | |
| var tooltip = ({ active, payload, _label }: { active: boolean, payload: { dataKey: string, value: number }[], _label: string }): React.JSX.Element | null => { |
Tools
Biome
[error] 81-81: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
| previewDataProvider: async (start: Date, end: Date) => { | ||
| var data = await getRandomFFWELData(start,end); | ||
| data[0].splice(data[0].length/2,1); | ||
| data[1].splice(data[1].length/2,1); | ||
| data[2].splice(data[2].length/2,1); | ||
| return Promise.resolve(data); | ||
| } |
There was a problem hiding this comment.
Handle data gaps more efficiently in ffwelLineChartWithDataGap.
- data[0].splice(data[0].length/2,1);
- data[1].splice(data[1].length/2,1);
- data[2].splice(data[2].length/2,1);
+ const midIndex = Math.floor(data[0].length / 2);
+ data.forEach(subArray => subArray.splice(midIndex, 1));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| previewDataProvider: async (start: Date, end: Date) => { | |
| var data = await getRandomFFWELData(start,end); | |
| data[0].splice(data[0].length/2,1); | |
| data[1].splice(data[1].length/2,1); | |
| data[2].splice(data[2].length/2,1); | |
| return Promise.resolve(data); | |
| } | |
| previewDataProvider: async (start: Date, end: Date) => { | |
| var data = await getRandomFFWELData(start,end); | |
| const midIndex = Math.floor(data[0].length / 2); | |
| data.forEach(subArray => subArray.splice(midIndex, 1)); | |
| return Promise.resolve(data); | |
| } |
| async function generateSurveyResponse(date: Date, resultIdentifier: string, surveyName: string, rangeStart: number, rangeEnd: number): Promise<SurveyAnswer> { | ||
| var answer = await predictableRandomNumber(getDayKey(date)+resultIdentifier); | ||
| return { | ||
| "id": "00000000-0000-0000-0000-000000000000", | ||
| "surveyID": "00000000-0000-0000-0000-000000000000", | ||
| "surveyResultID": "00000000-0000-0000-0000-000000000000", | ||
| "surveyVersion": 0, | ||
| "surveyName": surveyName, | ||
| "surveyDisplayName": surveyName, | ||
| "date": date.toISOString(), | ||
| "stepIdentifier": resultIdentifier, | ||
| "resultIdentifier": resultIdentifier, | ||
| "answers": [ | ||
| (answer % (rangeEnd - rangeStart) + rangeStart).toString() | ||
| ], | ||
| "insertedDate": date.toISOString() | ||
| }; | ||
| } |
There was a problem hiding this comment.
Refactor generateSurveyResponse to avoid using string literals for keys.
- return {
- "id": "00000000-0000-0000-0000-000000000000",
- "surveyID": "00000000-0000-0000-0000-000000000000",
- "surveyResultID": "00000000-0000-0000-0000-000000000000",
- "surveyVersion": 0,
- "surveyName": surveyName,
- "surveyDisplayName": surveyName,
- "date": date.toISOString(),
- "stepIdentifier": resultIdentifier,
- "resultIdentifier": resultIdentifier,
- "answers": [
- (answer % (rangeEnd - rangeStart) + rangeStart).toString()
- ],
- "insertedDate": date.toISOString()
- };
+ return {
+ id: "00000000-0000-0000-0000-000000000000",
+ surveyID: "00000000-0000-0000-0000-000000000000",
+ surveyResultID: "00000000-0000-0000-0000-000000000000",
+ surveyVersion: 0,
+ surveyName: surveyName,
+ surveyDisplayName: surveyName,
+ date: date.toISOString(),
+ stepIdentifier: resultIdentifier,
+ resultIdentifier: resultIdentifier,
+ answers: [
+ (answer % (rangeEnd - rangeStart) + rangeStart).toString()
+ ],
+ insertedDate: date.toISOString()
+ };Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| async function generateSurveyResponse(date: Date, resultIdentifier: string, surveyName: string, rangeStart: number, rangeEnd: number): Promise<SurveyAnswer> { | |
| var answer = await predictableRandomNumber(getDayKey(date)+resultIdentifier); | |
| return { | |
| "id": "00000000-0000-0000-0000-000000000000", | |
| "surveyID": "00000000-0000-0000-0000-000000000000", | |
| "surveyResultID": "00000000-0000-0000-0000-000000000000", | |
| "surveyVersion": 0, | |
| "surveyName": surveyName, | |
| "surveyDisplayName": surveyName, | |
| "date": date.toISOString(), | |
| "stepIdentifier": resultIdentifier, | |
| "resultIdentifier": resultIdentifier, | |
| "answers": [ | |
| (answer % (rangeEnd - rangeStart) + rangeStart).toString() | |
| ], | |
| "insertedDate": date.toISOString() | |
| }; | |
| } | |
| async function generateSurveyResponse(date: Date, resultIdentifier: string, surveyName: string, rangeStart: number, rangeEnd: number): Promise<SurveyAnswer> { | |
| var answer = await predictableRandomNumber(getDayKey(date)+resultIdentifier); | |
| return { | |
| id: "00000000-0000-0000-0000-000000000000", | |
| surveyID: "00000000-0000-0000-0000-000000000000", | |
| surveyResultID: "00000000-0000-0000-0000-000000000000", | |
| surveyVersion: 0, | |
| surveyName: surveyName, | |
| surveyDisplayName: surveyName, | |
| date: date.toISOString(), | |
| stepIdentifier: resultIdentifier, | |
| resultIdentifier: resultIdentifier, | |
| answers: [ | |
| (answer % (rangeEnd - rangeStart) + rangeStart).toString() | |
| ], | |
| insertedDate: date.toISOString() | |
| }; | |
| } |
Tools
Biome
[error] 15-15: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 16-16: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 17-17: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 18-18: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 19-19: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 20-20: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 21-21: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 22-22: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 23-23: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 24-24: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
[error] 27-27: The computed expression can be simplified without the use of a string literal. (lint/complexity/useLiteralKeys)
Unsafe fix: Use a literal key instead.
| )} | ||
| </defs> | ||
| {(props.options as MultiSeriesBarChartOptions)?.thresholds?.filter(t=>t.referenceLineColor)?.map((threshold, index) => | ||
| <ReferenceLine y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} /> |
There was a problem hiding this comment.
Add a key property to ReferenceLine to ensure stable React re-renders.
- <ReferenceLine y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} />
+ <ReferenceLine key={`ref-line-${index}`} y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} />Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <ReferenceLine y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} /> | |
| <ReferenceLine key={`ref-line-${index}`} y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} /> |
Tools
Biome
[error] 255-255: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
| expectedDataInterval?: Duration, | ||
| series: ChartSeries[] | AreaChartSeries[], | ||
| chartHasData: boolean, | ||
| tooltip: ({ active, payload, label }: any) => React.JSX.Element | null, |
There was a problem hiding this comment.
Specify a more precise type for the tooltip prop in TimeSeriesChartProps.
- tooltip: ({ active, payload, label }: any) => React.JSX.Element | null,
+ tooltip: ({ active, payload, label }: TooltipProps) => React.JSX.Element | null,Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 22-22: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
| title?: string | ||
| intervalType?: "Week" | "Month" | "6Month" | "Day", | ||
| intervalStart: Date, | ||
| data: Record<string,any>[] | undefined, |
There was a problem hiding this comment.
Specify a more precise type than any for data in TimeSeriesChartProps.
- data: Record<string,any>[] | undefined,
+ data: Record<string, number | string | Date>[] | undefined,Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| data: Record<string,any>[] | undefined, | |
| data: Record<string, number | string | Date>[] | undefined, |
Tools
Biome
[error] 18-18: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
| const DayTick = ({ x, y, stroke, payload }: any) => { | ||
| var value = payload.value; | ||
| let currentDate = new Date(value); | ||
| if (intervalType === "Month") { | ||
| return <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text>; | ||
| } else if (intervalType === "6Month" ){ | ||
| let monthLabel = currentDate.getDate() === 1 ? format(currentDate, "LLL") : ""; | ||
| let dayLabel = currentDate.getDate().toString(); | ||
| return <> | ||
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{monthLabel}</text> | ||
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{dayLabel}</text> | ||
| </>; | ||
| } else if (intervalType === "Week" ){ | ||
| let dayOfWeek: string = ""; | ||
| for (let i = 0; i < 7; i++) { | ||
| if (currentDate.getTime() === value) { | ||
| dayOfWeek = format(currentDate, "EEEEEE"); | ||
| break; | ||
| } | ||
| currentDate = add(currentDate, { days: 1 }); | ||
| } | ||
| return <> | ||
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{dayOfWeek}</text> | ||
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text> | ||
| </>; | ||
| } else if ( intervalType === "Day"){ | ||
| const startTime = new Date(props.intervalStart); | ||
| startTime.setHours(0, 0, 0, 0); | ||
| return <> | ||
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{format(currentDate, "hh:mm aaa")}</text> | ||
| </>; | ||
| } | ||
| return <> | ||
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{value}</text>; | ||
| </> | ||
| } |
There was a problem hiding this comment.
Refactor DayTick to avoid using any and improve type safety.
- const DayTick = ({ x, y, stroke, payload }: any) => {
+ interface DayTickProps {
+ x: number;
+ y: number;
+ stroke: string;
+ payload: { value: string };
+ }
+ const DayTick = ({ x, y, stroke, payload }: DayTickProps) => {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const DayTick = ({ x, y, stroke, payload }: any) => { | |
| var value = payload.value; | |
| let currentDate = new Date(value); | |
| if (intervalType === "Month") { | |
| return <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text>; | |
| } else if (intervalType === "6Month" ){ | |
| let monthLabel = currentDate.getDate() === 1 ? format(currentDate, "LLL") : ""; | |
| let dayLabel = currentDate.getDate().toString(); | |
| return <> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{monthLabel}</text> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{dayLabel}</text> | |
| </>; | |
| } else if (intervalType === "Week" ){ | |
| let dayOfWeek: string = ""; | |
| for (let i = 0; i < 7; i++) { | |
| if (currentDate.getTime() === value) { | |
| dayOfWeek = format(currentDate, "EEEEEE"); | |
| break; | |
| } | |
| currentDate = add(currentDate, { days: 1 }); | |
| } | |
| return <> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{dayOfWeek}</text> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text> | |
| </>; | |
| } else if ( intervalType === "Day"){ | |
| const startTime = new Date(props.intervalStart); | |
| startTime.setHours(0, 0, 0, 0); | |
| return <> | |
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{format(currentDate, "hh:mm aaa")}</text> | |
| </>; | |
| } | |
| return <> | |
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{value}</text>; | |
| </> | |
| } | |
| interface DayTickProps { | |
| x: number; | |
| y: number; | |
| stroke: string; | |
| payload: { value: string }; | |
| } | |
| const DayTick = ({ x, y, stroke, payload }: DayTickProps) => { | |
| var value = payload.value; | |
| let currentDate = new Date(value); | |
| if (intervalType === "Month") { | |
| return <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text>; | |
| } else if (intervalType === "6Month" ){ | |
| let monthLabel = currentDate.getDate() === 1 ? format(currentDate, "LLL") : ""; | |
| let dayLabel = currentDate.getDate().toString(); | |
| return <> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{monthLabel}</text> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{dayLabel}</text> | |
| </>; | |
| } else if (intervalType === "Week" ){ | |
| let dayOfWeek: string = ""; | |
| for (let i = 0; i < 7; i++) { | |
| if (currentDate.getTime() === value) { | |
| dayOfWeek = format(currentDate, "EEEEEE"); | |
| break; | |
| } | |
| currentDate = add(currentDate, { days: 1 }); | |
| } | |
| return <> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 8} textAnchor="middle" fontSize="11">{dayOfWeek}</text> | |
| <text className={isToday(currentDate) ? "today" : ""} fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{currentDate.getDate()}</text> | |
| </>; | |
| } else if ( intervalType === "Day"){ | |
| const startTime = new Date(props.intervalStart); | |
| startTime.setHours(0, 0, 0, 0); | |
| return <> | |
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 24} textAnchor="middle" fontSize="12">{format(currentDate, "hh:mm aaa")}</text> | |
| </>; | |
| } | |
| return <> | |
| <text fill="var(--mdhui-text-color-2)" x={x} y={y + 15} textAnchor="middle" fontSize="12">{value}</text>; | |
| </> | |
| } |
Tools
Biome
[error] 32-32: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
|
Do we anticipate a date yet for the next MDHDUI release and would this be included? |
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
Enhancements
TimeSeriesChartcomponent.Bug Fixes
Refactor