[FEATURE] Support Multi-Series Stat Chart #1241
Conversation
Signed-off-by: Eunice Wong <eunicewong@live.com>
19ab5f0 to
30f9a95
Compare
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
7ec9993 to
46f730d
Compare
| @@ -2281,19 +2281,17 @@ | |||
| "metadata": { | |||
| "name": "StatChartPanel", | |||
| "created_at": "2022-12-21T00:00:00Z", | |||
| "updated_at": "2023-01-27T00:35:26.333638Z", | |||
| "version": 8, | |||
| "updated_at": "2023-06-09T17:57:49.291171Z", | |||
There was a problem hiding this comment.
Updated testing/StatChartPanel dashboard with better mock data
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
Signed-off-by: Eunice Wong <eunicewong@live.com>
5426997 to
4eaf257
Compare
| text: data?.seriesData?.name ?? '', | ||
| fontWeight: SERIES_NAME_FONT_WEIGHT, | ||
| width, | ||
| height: height * 0.125, // assume series name will take 12.5% of available height |
There was a problem hiding this comment.
Out of curiosity, where did 12.5% come from? Is it an approximation or is it based on some specific math?
There was a problem hiding this comment.
from @foxbat07, all these percentages are just approximation for how we want the stat chart to look
| fontWeight: number; | ||
| width: number; | ||
| height: number; | ||
| lineHeight?: number; // default is 1.2 |
There was a problem hiding this comment.
You can use @default 1.2 to specify this if you put some typedoc above the lineHeight. I could see that being nice for folks using vscode and other tools that will show the typedoc on hover.
|
|
||
| use([EChartsLineChart, GridComponent, DatasetComponent, TitleComponent, TooltipComponent, CanvasRenderer]); | ||
|
|
||
| const MIN_VALUE_SIZE = 12; | ||
| const MAX_VALUE_SIZE = 36; | ||
| const LINE_HEIGHT = 1.2; |
There was a problem hiding this comment.
Is there any association between this constant and DEFAULT_LINE_HEIGHT in calculateFontSize.ts? If so, should one be set using the other?
There was a problem hiding this comment.
Actually yeah calculateFontSize is more accurate if using the line height from stat chart so updating this to removing DEFAULT_LINE_HEIGHT, thanks!
|
|
||
| function useCanvasContext() { | ||
| if (!canvasContext) { | ||
| canvasContext = document.createElement('canvas').getContext('2d'); |
There was a problem hiding this comment.
Do you happen to know if there's any perf issues with creating these over and over and not cleaning them up? I think JS is smart about garbage collecting them if they're never inserted into the dom, but it'd be worth double-checking.
There was a problem hiding this comment.
i think we should only be creating this once since canvasContext is defined globally
There was a problem hiding this comment.
If we are doing something global, I would make it pretty clear with the name + a comment. The naming convention seems like a hook with the use prefix, so maybe getGlobalCanvasContext() instead since no react hooks are used.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const noDataTextStyle = (chartsTheme.noDataOption.title as any).textStyle; |
There was a problem hiding this comment.
Out of curiosity, why is the any needed? Could you add a clarifying comment above the eslint-disable-line?
There was a problem hiding this comment.
chartsTheme.noDataOption is type EChartsCoreOption but EChartsCoreOption doesn't have a title field so getting Object is of type 'unknown' error :(
any idea if there is a better type for noDataOption?
There was a problem hiding this comment.
We should use ECharts ComposeOption actually, that would help with tooltip type weirdness too in LineChart, there is an example internally but something we can look into in separate PR later
There was a problem hiding this comment.
ComposeOption / minimal types docs, I had a branch where I started this I can try to find, just was going to take a bit of refactoring in LineChart but maybe easier to change in noDataOption / StatChart / GaugeChart
There was a problem hiding this comment.
oh awesome, added a new type NoDataOption that uses ComposeOption :)
Signed-off-by: Eunice Wong <eunicewong@live.com>
1e2ddb5 to
b0d8423
Compare
| const fontStyle = `${fontWeight} ${fontSize}px ${fontFamily}`; | ||
|
|
||
| // measure the width of the text with the given font style | ||
| const textMetrics: TextMetrics = ctx.measureText(text); |
Signed-off-by: Eunice Wong <eunicewong@live.com>
| const chartsTheme = useChartsTheme(); | ||
|
|
||
| if (queryResults[0]?.error) throw queryResults[0]?.error; | ||
|
|
||
| if (contentDimensions === undefined) return null; | ||
|
|
||
| if (isLoading === true) { | ||
| if (isLoading || isFetching) { |
There was a problem hiding this comment.
Nit: you should only need isFetching since it encapsulates isLoading. isLoading is only the first time
|
|
||
| function useCanvasContext() { | ||
| if (!canvasContext) { | ||
| canvasContext = document.createElement('canvas').getContext('2d'); |
There was a problem hiding this comment.
If we are doing something global, I would make it pretty clear with the name + a comment. The naming convention seems like a hook with the use prefix, so maybe getGlobalCanvasContext() instead since no react hooks are used.
Signed-off-by: Eunice Wong <eunicewong@live.com>
df25064 to
d13097f
Compare
This PR not only supports multi-series stat chart but also improves font size. Font size will now automatically grow to fit the available space.
stat-charts.mov
TO DO:
We want to add a manual option for adding your own font size as well.