Conversation
|
@gingerwizard is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
|
PR Review
Overall the implementation is solid, follows existing patterns (mirrors |
| // Validate bar charts require a group by | ||
| if ( | ||
| !isRawSqlChart && | ||
| form.displayType === DisplayType.Bar && | ||
| !form.groupBy | ||
| ) { | ||
| errors.push({ | ||
| path: `groupBy`, | ||
| message: 'Group By is required for bar charts', | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we add a unit test covering this check? There are existing unit tests covering this function which can be extended.
| > & { | ||
| maxNumberOfGroups?: number; | ||
| }; |
There was a problem hiding this comment.
Is this display setting intentionally not persisted? All other display settings are persisted as part of the config in the DB / URL Params and available via the API - it is odd if this one is not.
Perhaps instead of adding this new property, we can just use the existing limit property. That way it is persisted, and you don't have to copy back and forth between limit and maxNumberOfGroups in the places where that is happening right now.
normalizeChartConfig could also then be updated to strip limit from non-Bar type chart configs before saving, since the customizable limit is only available for bar charts (though it would be good to add to pie as well)
| </> | ||
| )} | ||
|
|
||
| {isBarChart && ( |
There was a problem hiding this comment.
This is neat, and would be nice to add to Pie charts too.
However, we'd ideally give the user an Order By input as well, so that they can specify which 10 groups to show. There is an order by on the table chart type that might be re-usable
There was a problem hiding this comment.
We also should not show this for raw sql bar charts, since it's ignored.
| ); | ||
| }; | ||
|
|
||
| export const DBBarChart = ({ |
There was a problem hiding this comment.
Please run make ci-lint or make dev-lint and fix the formatting errors that are output.
| numberFormat, | ||
| }: { | ||
| active?: boolean; | ||
| payload?: { name: string; value: number; payload: { color: string } }[]; |
There was a problem hiding this comment.
| payload?: { name: string; value: number; payload: { color: string } }[]; | |
| payload?: { name: string; value: number; payload: { color: string, label: string } }[]; |
| <ChartTooltipContainer> | ||
| <ChartTooltipItem | ||
| color={entry.payload.color} | ||
| name={entry.name} |
There was a problem hiding this comment.
I'd recommend using the payload's name here so that the full label shows in the tooltip (since the full label is potentially shortened below the x-axis.
| name={entry.name} | |
| name={entry.payload.name} |
| fillNulls, | ||
| compareToPreviousPeriod, | ||
| numberFormat, | ||
| maxNumberOfGroups: limit?.limit ?? 10, |
There was a problem hiding this comment.
nit: It would be good to create an re-use a constant DEFAULT_MAX_GROUPS = 10 value here and elsewhere where 10 has been hardcoded.
| export enum DisplayType { | ||
| Line = 'line', | ||
| StackedBar = 'stacked_bar', | ||
| Bar = 'bar', |
There was a problem hiding this comment.
The external dashboards APIs need to be updated for this new type as well. You'll see related errors when running make ci-lint:
src/routers/external-api/v2/utils/dashboards.ts:159:24 - error TS1360: Type 'DisplayType.Bar | undefined' does not satisfy the expected type 'undefined'.
Type 'DisplayType.Bar' is not assignable to type 'undefined'.
159 config.displayType satisfies never | undefined;
~~~~~~~~~
src/routers/external-api/v2/utils/dashboards.ts:261:26 - error TS1360: Type 'DisplayType.Bar' does not satisfy the expected type 'never'.
261 config.displayType satisfies never;
2 participants
Summary
This PR introduces Bar Charts. Historically, bar charts were effectively bar charts over time series. This implements true bar charts where the x-axis can be any grouping key.
As a result of this, line/bar charts are now called "Time Series"
Screenshots or video
How to test locally or on Vercel