Explore: Refactor ExploreGraph (#58660)

* WIP

* revert collapse changes

* use HorizontalGroup instead of custom styles

* fix tests

* use import aliases

public/app/core/utils/explore.ts

@@ -30,7 +30,7 @@ import { RefreshPicker } from '@grafana/ui';
 import store from 'app/core/store';
 import { TimeSrv } from 'app/features/dashboard/services/TimeSrv';
 import { PanelModel } from 'app/features/dashboard/state';
-import { EXPLORE_GRAPH_STYLES, ExploreGraphStyle, ExploreId, QueryOptions, QueryTransaction } from 'app/types/explore';
+import { ExploreId, QueryOptions, QueryTransaction } from 'app/types/explore';
 
 import { config } from '../config';
 
@@ -205,21 +205,6 @@ export const safeStringifyValue = (value: any, space?: number) => {
   return '';
 };
 
-const DEFAULT_GRAPH_STYLE: ExploreGraphStyle = 'lines';
-// we use this function to take any kind of data we loaded
-// from an external source (URL, localStorage, whatever),
-// and extract the graph-style from it, or return the default
-// graph-style if we are not able to do that.
-// it is important that this function is able to take any form of data,
-// (be it objects, or arrays, or booleans or whatever),
-// and produce a best-effort graphStyle.
-// note that typescript makes sure we make no mistake in this function.
-// we do not rely on ` as ` or ` any `.
-export const toGraphStyle = (data: unknown): ExploreGraphStyle => {
-  const found = EXPLORE_GRAPH_STYLES.find((v) => v === data);
-  return found ?? DEFAULT_GRAPH_STYLE;
-};
-
 export function parseUrlState(initial: string | undefined): ExploreUrlState {
   const parsed = safeParseJson(initial);
   const errorResult: any = {

public/app/features/explore/Explore.test.tsx

@@ -84,8 +84,6 @@ const dummyProps: Props = {
   showFlameGraph: true,
   splitOpen: (() => {}) as any,
   splitted: false,
-  changeGraphStyle: () => {},
-  graphStyle: 'lines',
   eventBus: new EventBusSrv(),
 };
 

public/app/features/explore/Explore.tsx

@@ -18,7 +18,7 @@ import {
 } from '@grafana/data';
 import { selectors } from '@grafana/e2e-selectors';
 import { config, getDataSourceSrv, reportInteraction } from '@grafana/runtime';
-import { Collapse, CustomScrollbar, ErrorBoundaryAlert, Themeable2, withTheme2, PanelContainer } from '@grafana/ui';
+import { CustomScrollbar, ErrorBoundaryAlert, Themeable2, withTheme2, PanelContainer } from '@grafana/ui';
 import { FILTER_FOR_OPERATOR, FILTER_OUT_OPERATOR, FilterItem } from '@grafana/ui/src/components/Table/types';
 import appEvents from 'app/core/app_events';
 import { supportedFeatures } from 'app/core/history/richHistoryStorageProvider';
@@ -26,15 +26,14 @@ import { MIXED_DATASOURCE_NAME } from 'app/plugins/datasource/mixed/MixedDataSou
 import { getNodeGraphDataFrames } from 'app/plugins/panel/nodeGraph/utils';
 import { StoreState } from 'app/types';
 import { AbsoluteTimeEvent } from 'app/types/events';
-import { ExploreGraphStyle, ExploreId, ExploreItemState } from 'app/types/explore';
+import { ExploreId, ExploreItemState } from 'app/types/explore';
 
 import { getTimeZone } from '../profile/state/selectors';
 
-import { ExploreGraph } from './ExploreGraph';
-import { ExploreGraphLabel } from './ExploreGraphLabel';
 import ExploreQueryInspector from './ExploreQueryInspector';
 import { ExploreToolbar } from './ExploreToolbar';
 import { FlameGraphExploreContainer } from './FlameGraphExploreContainer';
+import { GraphContainer } from './Graph/GraphContainer';
 import LogsContainer from './LogsContainer';
 import { NoData } from './NoData';
 import { NoDataSourceCallToAction } from './NoDataSourceCallToAction';
@@ -45,7 +44,7 @@ import RichHistoryContainer from './RichHistory/RichHistoryContainer';
 import { SecondaryActions } from './SecondaryActions';
 import TableContainer from './TableContainer';
 import { TraceViewContainer } from './TraceView/TraceViewContainer';
-import { changeSize, changeGraphStyle } from './state/explorePane';
+import { changeSize } from './state/explorePane';
 import { splitOpen } from './state/main';
 import { addQueryRow, modifyQueries, scanStart, scanStopAction, setQueries } from './state/query';
 import { isSplit } from './state/selectors';
@@ -222,11 +221,6 @@ export class Explore extends React.PureComponent<Props, ExploreState> {
     updateTimeRange({ exploreId, absoluteRange });
   };
 
-  onChangeGraphStyle = (graphStyle: ExploreGraphStyle) => {
-    const { exploreId, changeGraphStyle } = this.props;
-    changeGraphStyle(exploreId, graphStyle);
-  };
-
   toggleShowRichHistory = () => {
     this.setState((state) => {
       return {
@@ -277,28 +271,22 @@ export class Explore extends React.PureComponent<Props, ExploreState> {
   }
 
   renderGraphPanel(width: number) {
-    const { graphResult, absoluteRange, timeZone, queryResponse, loading, theme, graphStyle, showFlameGraph } =
-      this.props;
-    const spacing = parseInt(theme.spacing(2).slice(0, -2), 10);
-    const label = <ExploreGraphLabel graphStyle={graphStyle} onChangeGraphStyle={this.onChangeGraphStyle} />;
+    const { graphResult, absoluteRange, timeZone, queryResponse, loading, showFlameGraph } = this.props;
 
     return (
-      <Collapse label={label} loading={loading} isOpen>
-        <ExploreGraph
-          graphStyle={graphStyle}
-          data={graphResult!}
-          height={showFlameGraph ? 180 : 400}
-          width={width - spacing}
-          absoluteRange={absoluteRange}
-          onChangeTime={this.onUpdateTimeRange}
-          timeZone={timeZone}
-          annotations={queryResponse.annotations}
-          splitOpenFn={this.onSplitOpen('graph')}
-          loadingState={queryResponse.state}
-          anchorToZero={false}
-          eventBus={this.graphEventBus}
-        />
-      </Collapse>
+      <GraphContainer
+        loading={loading}
+        data={graphResult!}
+        height={showFlameGraph ? 180 : 400}
+        width={width}
+        absoluteRange={absoluteRange}
+        timeZone={timeZone}
+        onChangeTime={this.onUpdateTimeRange}
+        annotations={queryResponse.annotations}
+        splitOpenFn={this.onSplitOpen('graph')}
+        loadingState={queryResponse.state}
+        eventBus={this.graphEventBus}
+      />
     );
   }
 
@@ -515,7 +503,6 @@ function mapStateToProps(state: StoreState, { exploreId }: ExploreProps) {
     showNodeGraph,
     showFlameGraph,
     loading,
-    graphStyle,
   } = item;
 
   return {
@@ -538,13 +525,11 @@ function mapStateToProps(state: StoreState, { exploreId }: ExploreProps) {
     showFlameGraph,
     splitted: isSplit(state),
     loading,
-    graphStyle,
   };
 }
 
 const mapDispatchToProps = {
   changeSize,
-  changeGraphStyle,
   modifyQueries,
   scanStart,
   scanStopAction,

public/app/features/explore/Graph/ExploreGraph.tsx

@@ -31,9 +31,9 @@ import {
 } from '@grafana/ui';
 import { defaultGraphConfig, getGraphFieldConfig } from 'app/plugins/panel/timeseries/config';
 import { TimeSeriesOptions } from 'app/plugins/panel/timeseries/types';
+import { ExploreGraphStyle } from 'app/types';
 
-import { ExploreGraphStyle } from '../../types';
-import { seriesVisibilityConfigFactory } from '../dashboard/dashgrid/SeriesVisibilityConfigFactory';
+import { seriesVisibilityConfigFactory } from '../../dashboard/dashgrid/SeriesVisibilityConfigFactory';
 
 import { applyGraphStyle } from './exploreGraphStyleUtils';
 
@@ -52,7 +52,7 @@ interface Props {
   splitOpenFn: SplitOpen;
   onChangeTime: (timeRange: AbsoluteTimeRange) => void;
   graphStyle: ExploreGraphStyle;
-  anchorToZero: boolean;
+  anchorToZero?: boolean;
   eventBus: EventBus;
 }
 
@@ -69,13 +69,14 @@ export function ExploreGraph({
   splitOpenFn,
   graphStyle,
   tooltipDisplayMode = TooltipDisplayMode.Single,
-  anchorToZero,
+  anchorToZero = false,
   eventBus,
 }: Props) {
   const theme = useTheme2();
   const style = useStyles2(getStyles);
   const [showAllTimeSeries, setShowAllTimeSeries] = useState(false);
-  const [structureRev, { inc: incrementStructureRev }] = useCounter(1);
+  const [structureRev, { inc }] = useCounter(0);
+
   const fieldConfigRegistry = useMemo(
     () => createFieldConfigRegistry(getGraphFieldConfig(defaultGraphConfig), 'Explore'),
     []
@@ -118,12 +119,10 @@ export function ExploreGraph({
     });
   }, [fieldConfigRegistry, data, timeZone, theme, styledFieldConfig]);
 
-  // structureRev should be incremented when either the number of series or the config changes.
-  // like useEffect, but runs before rendering.
-  // TODO: while this works as it is supposed to, we are forced to do this now because of the way
-  // ExploreGraph is implemented. We should refactor it to a single component that handles structureRev increments
-  // when a user changes the viz style and not react to the value change itself.
-  useMemo(incrementStructureRev, [dataWithConfig.length, styledFieldConfig, incrementStructureRev]);
+  // We need to increment structureRev when the number of series changes.
+  // the function passed to useMemo runs during rendering, so when we get a different
+  // amount of data, structureRev is incremented before we render it
+  useMemo(inc, [dataWithConfig.length, styledFieldConfig, inc]);
 
   useEffect(() => {
     if (onHiddenSeriesChanged) {

public/app/features/explore/Graph/ExploreGraphLabel.tsx

@@ -1,10 +1,8 @@
-import { css } from '@emotion/css';
 import React from 'react';
 
 import { SelectableValue } from '@grafana/data';
-import { RadioButtonGroup } from '@grafana/ui';
-
-import { EXPLORE_GRAPH_STYLES, ExploreGraphStyle } from '../../types';
+import { RadioButtonGroup, HorizontalGroup } from '@grafana/ui';
+import { EXPLORE_GRAPH_STYLES, ExploreGraphStyle } from 'app/types';
 
 const ALL_GRAPH_STYLE_OPTIONS: Array<SelectableValue<ExploreGraphStyle>> = EXPLORE_GRAPH_STYLES.map((style) => ({
   value: style,
@@ -12,11 +10,6 @@ const ALL_GRAPH_STYLE_OPTIONS: Array<SelectableValue<ExploreGraphStyle>> = EXPLO
   label: style[0].toUpperCase() + style.slice(1).replace(/_/, ' '),
 }));
 
-const spacing = css({
-  display: 'flex',
-  justifyContent: 'space-between',
-});
-
 type Props = {
   graphStyle: ExploreGraphStyle;
   onChangeGraphStyle: (style: ExploreGraphStyle) => void;
@@ -25,9 +18,9 @@ type Props = {
 export function ExploreGraphLabel(props: Props) {
   const { graphStyle, onChangeGraphStyle } = props;
   return (
-    <div className={spacing}>
+    <HorizontalGroup justify="space-between" wrap>
       Graph
       <RadioButtonGroup size="sm" options={ALL_GRAPH_STYLE_OPTIONS} value={graphStyle} onChange={onChangeGraphStyle} />
-    </div>
+    </HorizontalGroup>
   );
 }

public/app/features/explore/Graph/GraphContainer.tsx

@@ -0,0 +1,70 @@
+import React, { useCallback, useState } from 'react';
+
+import { DataFrame, EventBus, AbsoluteTimeRange, TimeZone, SplitOpen, LoadingState } from '@grafana/data';
+import { Collapse, useTheme2 } from '@grafana/ui';
+import { ExploreGraphStyle } from 'app/types';
+
+import { storeGraphStyle } from '../state/utils';
+
+import { ExploreGraph } from './ExploreGraph';
+import { ExploreGraphLabel } from './ExploreGraphLabel';
+import { loadGraphStyle } from './utils';
+
+interface Props {
+  loading: boolean;
+  data: DataFrame[];
+  annotations?: DataFrame[];
+  eventBus: EventBus;
+  height: number;
+  width: number;
+  absoluteRange: AbsoluteTimeRange;
+  timeZone: TimeZone;
+  onChangeTime: (absoluteRange: AbsoluteTimeRange) => void;
+  splitOpenFn: SplitOpen;
+  loadingState: LoadingState;
+}
+
+export const GraphContainer = ({
+  loading,
+  data,
+  eventBus,
+  height,
+  width,
+  absoluteRange,
+  timeZone,
+  annotations,
+  onChangeTime,
+  splitOpenFn,
+  loadingState,
+}: Props) => {
+  const [graphStyle, setGraphStyle] = useState(loadGraphStyle);
+  const theme = useTheme2();
+  const spacing = parseInt(theme.spacing(2).slice(0, -2), 10);
+
+  const onGraphStyleChange = useCallback((graphStyle: ExploreGraphStyle) => {
+    storeGraphStyle(graphStyle);
+    setGraphStyle(graphStyle);
+  }, []);
+
+  return (
+    <Collapse
+      label={<ExploreGraphLabel graphStyle={graphStyle} onChangeGraphStyle={onGraphStyleChange} />}
+      loading={loading}
+      isOpen
+    >
+      <ExploreGraph
+        graphStyle={graphStyle}
+        data={data}
+        height={height}
+        width={width - spacing}
+        absoluteRange={absoluteRange}
+        onChangeTime={onChangeTime}
+        timeZone={timeZone}
+        annotations={annotations}
+        splitOpenFn={splitOpenFn}
+        loadingState={loadingState}
+        eventBus={eventBus}
+      />
+    </Collapse>
+  );
+};

public/app/features/explore/Graph/exploreGraphStyleUtils.ts

@@ -2,8 +2,7 @@ import produce from 'immer';
 
 import { FieldConfigSource } from '@grafana/data';
 import { GraphDrawStyle, GraphFieldConfig, StackingMode } from '@grafana/schema';
-
-import { ExploreGraphStyle } from '../../types';
+import { ExploreGraphStyle } from 'app/types';
 
 export type FieldConfig = FieldConfigSource<GraphFieldConfig>;
 

public/app/features/explore/Graph/utils.ts

@@ -0,0 +1,26 @@
+import store from 'app/core/store';
+import { ExploreGraphStyle, EXPLORE_GRAPH_STYLES } from 'app/types';
+
+const GRAPH_STYLE_KEY = 'grafana.explore.style.graph';
+export const storeGraphStyle = (graphStyle: string): void => {
+  store.set(GRAPH_STYLE_KEY, graphStyle);
+};
+
+export const loadGraphStyle = (): ExploreGraphStyle => {
+  return toGraphStyle(store.get(GRAPH_STYLE_KEY));
+};
+
+const DEFAULT_GRAPH_STYLE: ExploreGraphStyle = 'lines';
+// we use this function to take any kind of data we loaded
+// from an external source (URL, localStorage, whatever),
+// and extract the graph-style from it, or return the default
+// graph-style if we are not able to do that.
+// it is important that this function is able to take any form of data,
+// (be it objects, or arrays, or booleans or whatever),
+// and produce a best-effort graphStyle.
+// note that typescript makes sure we make no mistake in this function.
+// we do not rely on ` as ` or ` any `.
+export const toGraphStyle = (data: unknown): ExploreGraphStyle => {
+  const found = EXPLORE_GRAPH_STYLES.find((v) => v === data);
+  return found ?? DEFAULT_GRAPH_STYLE;
+};

public/app/features/explore/LogsVolumePanel.test.tsx

@@ -5,7 +5,7 @@ import { DataQueryResponse, LoadingState, EventBusSrv } from '@grafana/data';
 
 import { LogsVolumePanel } from './LogsVolumePanel';
 
-jest.mock('./ExploreGraph', () => {
+jest.mock('./Graph/ExploreGraph', () => {
   const ExploreGraph = () => <span>ExploreGraph</span>;
   return {
     ExploreGraph,

public/app/features/explore/LogsVolumePanel.tsx

@@ -13,7 +13,7 @@ import {
 } from '@grafana/data';
 import { Alert, Button, Collapse, InlineField, TooltipDisplayMode, useStyles2, useTheme2 } from '@grafana/ui';
 
-import { ExploreGraph } from './ExploreGraph';
+import { ExploreGraph } from './Graph/ExploreGraph';
 
 type Props = {
   logsVolumeData: DataQueryResponse | undefined;
@@ -125,7 +125,7 @@ export function LogsVolumePanel(props: Props) {
           loadingState={LoadingState.Done}
           data={logsVolumeData.data}
           height={height}
-          width={width - spacing}
+          width={width - spacing * 2}
           absoluteRange={range}
           onChangeTime={onUpdateTimeRange}
           timeZone={timeZone}

public/app/features/explore/state/explorePane.ts

@@ -24,7 +24,7 @@ import {
 } from 'app/core/utils/explore';
 import { getFiscalYearStartMonth, getTimeZone } from 'app/features/profile/state/selectors';
 import { ThunkResult } from 'app/types';
-import { ExploreGraphStyle, ExploreId, ExploreItemState } from 'app/types/explore';
+import { ExploreId, ExploreItemState } from 'app/types/explore';
 
 import { datasourceReducer } from './datasource';
 import { historyReducer } from './history';
@@ -36,7 +36,6 @@ import {
   loadAndInitDatasource,
   createEmptyQueryResponse,
   getUrlStateFromPaneState,
-  storeGraphStyle,
 } from './utils';
 // Types
 
@@ -118,20 +117,6 @@ export function changeSize(
   return changeSizeAction({ exploreId, height, width });
 }
 
-interface ChangeGraphStylePayload {
-  exploreId: ExploreId;
-  graphStyle: ExploreGraphStyle;
-}
-
-const changeGraphStyleAction = createAction<ChangeGraphStylePayload>('explore/changeGraphStyle');
-
-export function changeGraphStyle(exploreId: ExploreId, graphStyle: ExploreGraphStyle): ThunkResult<void> {
-  return async (dispatch, getState) => {
-    storeGraphStyle(graphStyle);
-    dispatch(changeGraphStyleAction({ exploreId, graphStyle }));
-  };
-}
-
 /**
  * Initialize Explore state with state from the URL and the React component.
  * Call this only on components for with the Explore state has not been initialized.
@@ -281,11 +266,6 @@ export const paneReducer = (state: ExploreItemState = makeExplorePaneState(), ac
     return { ...state, containerWidth };
   }
 
-  if (changeGraphStyleAction.match(action)) {
-    const { graphStyle } = action.payload;
-    return { ...state, graphStyle };
-  }
-
   if (changePanelsStateAction.match(action)) {
     const { panelsState } = action.payload;
     return { ...state, panelsState };

public/app/features/explore/state/utils.ts

@@ -12,10 +12,10 @@ import {
   PanelData,
 } from '@grafana/data';
 import { ExplorePanelData } from 'app/types';
-import { ExploreGraphStyle, ExploreItemState } from 'app/types/explore';
+import { ExploreItemState } from 'app/types/explore';
 
 import store from '../../../core/store';
-import { clearQueryKeys, lastUsedDatasourceKeyForOrgId, toGraphStyle } from '../../../core/utils/explore';
+import { clearQueryKeys, lastUsedDatasourceKeyForOrgId } from '../../../core/utils/explore';
 import { getDatasourceSrv } from '../../plugins/datasource_srv';
 import { SETTINGS_KEYS } from '../utils/logs';
 import { toRawTimeRange } from '../utils/time';
@@ -30,11 +30,6 @@ export const storeGraphStyle = (graphStyle: string): void => {
   store.set(GRAPH_STYLE_KEY, graphStyle);
 };
 
-const loadGraphStyle = (): ExploreGraphStyle => {
-  const data = store.get(GRAPH_STYLE_KEY);
-  return toGraphStyle(data);
-};
-
 const LOGS_VOLUME_ENABLED_KEY = SETTINGS_KEYS.enableVolumeHistogram;
 export const storeLogsVolumeEnabled = (enabled: boolean): void => {
   store.set(LOGS_VOLUME_ENABLED_KEY, enabled ? 'true' : 'false');
@@ -84,7 +79,6 @@ export const makeExplorePaneState = (): ExploreItemState => ({
   logsVolumeEnabled: loadLogsVolumeEnabled(),
   logsVolumeDataProvider: undefined,
   logsVolumeData: undefined,
-  graphStyle: loadGraphStyle(),
   panelsState: {},
 });
 

public/app/types/explore.ts

@@ -187,8 +187,6 @@ export interface ExploreItemState {
   logsVolumeDataSubscription?: SubscriptionLike;
   logsVolumeData?: DataQueryResponse;
 
-  /* explore graph style */
-  graphStyle: ExploreGraphStyle;
   panelsState: ExplorePanelsState;
 }