adding watch visualization#35404
Conversation
💔 Build Failed |
💚 Build Succeeded |
|
Pinging @elastic/es-ui |
alisonelizabeth
left a comment
There was a problem hiding this comment.
@bmcconaghy nice work!
I tested locally. Not sure if I hit all the possible scenarios, but here are a couple things I noticed:
-
Tooltip missing when you hover over data points.
-
What do you think about having a chart loading state? Without it, I noticed the
EuiCallOutsometimes appears briefly when changing field values. -
In the old UI, when you select
Grouped over, it looks like the UI switches to a slider-type presentation and multiple charts are rendered. I don’t think there’s support for that yet here, so instead theEuiCallOutmessage renders.
Old UI:
x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx
Show resolved
Hide resolved
| colorValues: [], | ||
| specId: getSpecId('threshold'), | ||
| }; | ||
| thresholdCustomSeriesColors.set(thresholdDataSeriesColorValues, 'red'); |
There was a problem hiding this comment.
nit: should we use the hex from the eui color pallete? it's #BD271E
There was a problem hiding this comment.
makes sense, done
| /> | ||
| </Chart> | ||
| ) : ( | ||
| <EuiCallOut title="No data" color="warning"> |
There was a problem hiding this comment.
i18n for title here?
There was a problem hiding this comment.
good catch, added
cjcenizal
left a comment
There was a problem hiding this comment.
Looks great! Just had a couple suggestions and a question.
| return detectedTimezone; | ||
| } | ||
|
|
||
| const tzOffset = moment().format('Z'); |
There was a problem hiding this comment.
What does this do? Could we add a comment explaining the intention behind this?
| specId: getSpecId('threshold'), | ||
| }; | ||
| thresholdCustomSeriesColors.set(thresholdDataSeriesColorValues, 'red'); | ||
| const theme = { |
There was a problem hiding this comment.
To make this work with dark theme, I think we should import DARK_THEME from the charts lib and extract this out into a helper function near the top of the file:
const getChartTheme = () => {
const isDarkTheme = chrome.getUiSettingsClient().get('theme:darkMode');
const baseTheme = isDarkTheme ? DARK_THEME : LIGHT_THEME;
return {
...baseTheme,
lineSeriesStyle: {
...baseTheme.lineSeriesStyle,
line: {
...baseTheme.lineSeriesStyle.line,
strokeWidth: 3,
},
point: {
...baseTheme.lineSeriesStyle.point,
visible: false,
},
},
};
};There was a problem hiding this comment.
good point, forgot about dark mode, done
|
|
||
| const WatchVisualizationUi = () => { | ||
| const { watch } = useContext(WatchContext); | ||
| const [watchVisualizationData, setWatchVisualizationData] = useState<number[][]>([]); |
There was a problem hiding this comment.
It doesn't have a functional impact, but in case you find it cleaner you could also extract this and the effect on line 95 into a custom hook:
const useWatchVisualizationData = watch => {
const [watchVisualizationData, setWatchVisualizationData] = useState<number[][]>([]);
useEffect(
() => {
loadWatchVisualizationData(watch, setWatchVisualizationData);
},
[watch]
);
return watchVisualizationData;
}Then consuming it would look like this:
const WatchVisualizationUi = () => {
const { watch } = useContext(WatchContext);
const watchVisualizationData = useWatchVisualizationData(watch);
/* ... */
}There was a problem hiding this comment.
I think it prefer it in this way personally.
|
@alisonelizabeth the tooltip missing is a bug in EUI charts. If the data is continuous, they appear, if not, not. Chart loading state makes sense, but maybe that can be done in a separate PR. The paged charts thing just got implemented as a chart with multiple lines. This works pretty well for lower values of top N, but is obviously not great with higher values. Will probably need a revisit to sort out the proper UI here. |
💔 Build Failed |
| import { aggTypes } from '../../../models/watch/agg_types'; | ||
| const getChartTheme = () => { | ||
| const isDarkTheme = chrome.getUiSettingsClient().get('theme:darkMode'); | ||
| const baseTheme = isDarkTheme ? DARK_THEME : LIGHT_THEME; |
There was a problem hiding this comment.
I think you need to import DARK_THEME from @elastic/charts
|
@bmcconaghy thanks for making the changes! I'm getting an error with the latest. Do you think you could take a look? |
|
@alisonelizabeth not able to reproduce that error, maybe we can pair and you can show it to me? I did find a React warning (missing key on array) that I pushed a fix for. |
💔 Build Failed |
|
@bmcconaghy thanks for looking into it. i believe the error is something with my local dev env. latest changes LGTM. |
💚 Build Succeeded |
This PR adds a visualization when the threshold watch conditions are valid.
A couple of things need to get figured out: