From faf7cb93127b9b39a87db84d094cc76263020e98 Mon Sep 17 00:00:00 2001 From: ismail simsek Date: Sun, 11 Aug 2024 23:48:36 +0200 Subject: [PATCH] Chore: Refactor explore metrics layout switcher and breakdown scene (#90944) * refactor breakdown scene * refactor BreakdownScene along with LayoutSwitcher * rename * don't pass default layout * better type handling * betterer --- .betterer.results | 5 +- .../trails/ActionTabs/BreakdownScene.tsx | 34 ++++---- .../trails/ActionTabs/LayoutSwitcher.tsx | 81 ++++++++++++------- .../app/features/trails/ActionTabs/types.ts | 12 ++- public/app/features/trails/MetricScene.tsx | 28 ++----- public/app/features/trails/interactions.ts | 4 +- 6 files changed, 93 insertions(+), 71 deletions(-) diff --git a/.betterer.results b/.betterer.results index 68d2a6b0b03..73851418191 100644 --- a/.betterer.results +++ b/.betterer.results @@ -5441,9 +5441,8 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "2"] ], "public/app/features/trails/MetricScene.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "1"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "2"] + [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] ], "public/app/features/trails/MetricSelect/MetricSelectScene.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], diff --git a/public/app/features/trails/ActionTabs/BreakdownScene.tsx b/public/app/features/trails/ActionTabs/BreakdownScene.tsx index 564506f066f..d5b742308d0 100644 --- a/public/app/features/trails/ActionTabs/BreakdownScene.tsx +++ b/public/app/features/trails/ActionTabs/BreakdownScene.tsx @@ -37,6 +37,7 @@ import { AddToFiltersGraphAction } from './AddToFiltersGraphAction'; import { ByFrameRepeater } from './ByFrameRepeater'; import { LayoutSwitcher } from './LayoutSwitcher'; import { breakdownPanelOptions } from './panelConfigs'; +import { BreakdownLayoutChangeCallback, BreakdownLayoutType } from './types'; import { getLabelOptions } from './utils'; import { BreakdownAxisChangeEvent, yAxisSyncBehavior } from './yAxisSyncBehavior'; @@ -97,13 +98,6 @@ export class BreakdownScene extends SceneObjectBase { this.clearBreakdownPanelAxisValues(); }); - metricScene.subscribeToState(({ layout }, old) => { - if (layout !== old.layout) { - // Change in layout will set up a different set of panel objects that haven't received the current yaxis range - this.clearBreakdownPanelAxisValues(); - } - }); - this.updateBody(variable); } @@ -186,8 +180,8 @@ export class BreakdownScene extends SceneObjectBase { if (!variable.state.loading && variable.state.options.length) { stateUpdate.body = variable.hasAllValue() - ? buildAllLayout(options, this._query!) - : buildNormalLayout(this._query!); + ? buildAllLayout(options, this._query!, this.onBreakdownLayoutChange) + : buildNormalLayout(this._query!, this.onBreakdownLayoutChange); } else if (!variable.state.loading) { stateUpdate.body = undefined; stateUpdate.blockingMessage = 'Unable to retrieve label options for currently selected metric.'; @@ -198,6 +192,10 @@ export class BreakdownScene extends SceneObjectBase { this.setState(stateUpdate); } + public onBreakdownLayoutChange = (_: BreakdownLayoutType) => { + this.clearBreakdownPanelAxisValues(); + }; + public onChange = (value?: string) => { if (!value) { return; @@ -271,7 +269,11 @@ function getStyles(theme: GrafanaTheme2) { }; } -export function buildAllLayout(options: Array>, queryDef: AutoQueryDef) { +export function buildAllLayout( + options: Array>, + queryDef: AutoQueryDef, + onBreakdownLayoutChange: BreakdownLayoutChangeCallback +) { const children: SceneFlexItemLike[] = []; for (const option of options) { @@ -318,11 +320,12 @@ export function buildAllLayout(options: Array>, queryDef ); } return new LayoutSwitcher({ - options: [ + breakdownLayoutOptions: [ { value: 'grid', label: 'Grid' }, { value: 'rows', label: 'Rows' }, ], - layouts: [ + onBreakdownLayoutChange, + breakdownLayouts: [ new SceneCSSGridLayout({ templateColumns: GRID_TEMPLATE_COLUMNS, autoRows: '200px', @@ -342,7 +345,7 @@ export function buildAllLayout(options: Array>, queryDef const GRID_TEMPLATE_COLUMNS = 'repeat(auto-fit, minmax(400px, 1fr))'; -function buildNormalLayout(queryDef: AutoQueryDef) { +function buildNormalLayout(queryDef: AutoQueryDef, onBreakdownLayoutChange: BreakdownLayoutChangeCallback) { const unit = queryDef.unit; function getLayoutChild(data: PanelData, frame: DataFrame, frameIndex: number): SceneFlexItem { @@ -377,12 +380,13 @@ function buildNormalLayout(queryDef: AutoQueryDef) { maxDataPoints: 300, queries: queryDef.queries, }), - options: [ + breakdownLayoutOptions: [ { value: 'single', label: 'Single' }, { value: 'grid', label: 'Grid' }, { value: 'rows', label: 'Rows' }, ], - layouts: [ + onBreakdownLayoutChange, + breakdownLayouts: [ new SceneFlexLayout({ direction: 'column', children: [ diff --git a/public/app/features/trails/ActionTabs/LayoutSwitcher.tsx b/public/app/features/trails/ActionTabs/LayoutSwitcher.tsx index b1f621b03c7..6c2bd614643 100644 --- a/public/app/features/trails/ActionTabs/LayoutSwitcher.tsx +++ b/public/app/features/trails/ActionTabs/LayoutSwitcher.tsx @@ -1,58 +1,85 @@ import { SelectableValue } from '@grafana/data'; -import { SceneComponentProps, sceneGraph, SceneObject, SceneObjectBase, SceneObjectState } from '@grafana/scenes'; +import { + SceneComponentProps, + SceneObject, + SceneObjectBase, + SceneObjectState, + SceneObjectUrlSyncConfig, + SceneObjectUrlValues, + SceneObjectWithUrlSync, +} from '@grafana/scenes'; import { Field, RadioButtonGroup } from '@grafana/ui'; -import { MetricScene } from '../MetricScene'; import { reportExploreMetrics } from '../interactions'; -import { TRAIL_BREAKDOWN_VIEW_KEY } from '../shared'; +import { MakeOptional, TRAIL_BREAKDOWN_VIEW_KEY } from '../shared'; -import { LayoutType } from './types'; +import { isBreakdownLayoutType, BreakdownLayoutChangeCallback, BreakdownLayoutType } from './types'; export interface LayoutSwitcherState extends SceneObjectState { - layouts: SceneObject[]; - options: Array>; + activeBreakdownLayout: BreakdownLayoutType; + breakdownLayouts: SceneObject[]; + breakdownLayoutOptions: Array>; + onBreakdownLayoutChange: BreakdownLayoutChangeCallback; } -export class LayoutSwitcher extends SceneObjectBase { - private getMetricScene() { - return sceneGraph.getAncestor(this, MetricScene); +export class LayoutSwitcher extends SceneObjectBase implements SceneObjectWithUrlSync { + protected _urlSync = new SceneObjectUrlSyncConfig(this, { keys: ['breakdownLayout'] }); + + public constructor(state: MakeOptional) { + const storedBreakdownLayout = localStorage.getItem(TRAIL_BREAKDOWN_VIEW_KEY); + super({ + activeBreakdownLayout: isBreakdownLayoutType(storedBreakdownLayout) ? storedBreakdownLayout : 'grid', + ...state, + }); + } + + getUrlState() { + return { breakdownLayout: this.state.activeBreakdownLayout }; + } + + updateFromUrl(values: SceneObjectUrlValues) { + const newBreakdownLayout = values.breakdownLayout; + if (newBreakdownLayout === 'string' && isBreakdownLayoutType(newBreakdownLayout)) { + if (this.state.activeBreakdownLayout !== newBreakdownLayout) { + this.setState({ activeBreakdownLayout: newBreakdownLayout }); + } + } } public Selector({ model }: { model: LayoutSwitcher }) { - const { options } = model.useState(); - const activeLayout = model.useActiveLayout(); + const { activeBreakdownLayout, breakdownLayoutOptions } = model.useState(); return ( - + ); } - private useActiveLayout() { - const { options } = this.useState(); - const { layout } = this.getMetricScene().useState(); + public onLayoutChange = (active: BreakdownLayoutType) => { + if (this.state.activeBreakdownLayout === active) { + return; + } - const activeLayout = options.map((option) => option.value).includes(layout) ? layout : options[0].value; - return activeLayout; - } - - public onLayoutChange = (layout: LayoutType) => { - reportExploreMetrics('breakdown_layout_changed', { layout }); - localStorage.setItem(TRAIL_BREAKDOWN_VIEW_KEY, layout); - this.getMetricScene().setState({ layout }); + reportExploreMetrics('breakdown_layout_changed', { layout: active }); + localStorage.setItem(TRAIL_BREAKDOWN_VIEW_KEY, active); + this.setState({ activeBreakdownLayout: active }); + this.state.onBreakdownLayoutChange(active); }; public static Component = ({ model }: SceneComponentProps) => { - const { layouts, options } = model.useState(); - const activeLayout = model.useActiveLayout(); + const { breakdownLayouts, breakdownLayoutOptions, activeBreakdownLayout } = model.useState(); - const index = options.findIndex((o) => o.value === activeLayout); + const index = breakdownLayoutOptions.findIndex((o) => o.value === activeBreakdownLayout); if (index === -1) { return null; } - const layout = layouts[index]; + const layout = breakdownLayouts[index]; return ; }; diff --git a/public/app/features/trails/ActionTabs/types.ts b/public/app/features/trails/ActionTabs/types.ts index d65482c4a29..4ee5fa1c5ee 100644 --- a/public/app/features/trails/ActionTabs/types.ts +++ b/public/app/features/trails/ActionTabs/types.ts @@ -1,7 +1,11 @@ -const LAYOUT_TYPES = ['single', 'grid', 'rows'] as const; +const BREAKDOWN_LAYOUT_TYPES = ['single', 'grid', 'rows'] as const; -export type LayoutType = (typeof LAYOUT_TYPES)[number]; +export type BreakdownLayoutType = (typeof BREAKDOWN_LAYOUT_TYPES)[number]; -export function isLayoutType(layoutType: string | null | undefined): layoutType is LayoutType { - return !!layoutType && layoutType in LAYOUT_TYPES; +export function isBreakdownLayoutType( + breakdownLayoutType: string | null | undefined +): breakdownLayoutType is BreakdownLayoutType { + return !!breakdownLayoutType && breakdownLayoutType in BREAKDOWN_LAYOUT_TYPES; } + +export type BreakdownLayoutChangeCallback = (newBreakdownLayout: BreakdownLayoutType) => void; diff --git a/public/app/features/trails/MetricScene.tsx b/public/app/features/trails/MetricScene.tsx index 65c43546b20..698207e53e5 100644 --- a/public/app/features/trails/MetricScene.tsx +++ b/public/app/features/trails/MetricScene.tsx @@ -3,23 +3,22 @@ import { css } from '@emotion/css'; import { GrafanaTheme2 } from '@grafana/data'; import { config } from '@grafana/runtime'; import { - SceneObjectState, - SceneObjectBase, + QueryVariable, SceneComponentProps, + sceneGraph, + SceneObjectBase, + SceneObjectState, SceneObjectUrlSyncConfig, SceneObjectUrlValues, - sceneGraph, SceneVariableSet, - QueryVariable, } from '@grafana/scenes'; -import { ToolbarButton, Box, Stack, Icon, TabsBar, Tab, useStyles2, LinkButton, Tooltip } from '@grafana/ui'; +import { Box, Icon, LinkButton, Stack, Tab, TabsBar, ToolbarButton, Tooltip, useStyles2 } from '@grafana/ui'; import { getExploreUrl } from '../../core/utils/explore'; import { buildBreakdownActionScene } from './ActionTabs/BreakdownScene'; import { buildMetricOverviewScene } from './ActionTabs/MetricOverviewScene'; import { buildRelatedMetricsScene } from './ActionTabs/RelatedMetricsScene'; -import { isLayoutType, LayoutType } from './ActionTabs/types'; import { getAutoQueriesForMetric } from './AutomaticMetricQueries/AutoQueryEngine'; import { AutoQueryDef, AutoQueryInfo } from './AutomaticMetricQueries/types'; import { MAIN_PANEL_MAX_HEIGHT, MAIN_PANEL_MIN_HEIGHT, MetricGraphScene } from './MetricGraphScene'; @@ -32,7 +31,6 @@ import { getVariablesWithMetricConstant, MakeOptional, MetricSelectedEvent, - TRAIL_BREAKDOWN_VIEW_KEY, trailDS, VAR_GROUP_BY, VAR_METRIC_EXPR, @@ -43,24 +41,21 @@ export interface MetricSceneState extends SceneObjectState { body: MetricGraphScene; metric: string; actionView?: string; - layout: LayoutType; autoQuery: AutoQueryInfo; queryDef?: AutoQueryDef; } export class MetricScene extends SceneObjectBase { - protected _urlSync = new SceneObjectUrlSyncConfig(this, { keys: ['actionView', 'layout'] }); + protected _urlSync = new SceneObjectUrlSyncConfig(this, { keys: ['actionView'] }); - public constructor(state: MakeOptional) { + public constructor(state: MakeOptional) { const autoQuery = state.autoQuery ?? getAutoQueriesForMetric(state.metric); - const layout = localStorage.getItem(TRAIL_BREAKDOWN_VIEW_KEY); super({ $variables: state.$variables ?? getVariableSet(state.metric), body: state.body ?? new MetricGraphScene({}), autoQuery, queryDef: state.queryDef ?? autoQuery.main, - layout: isLayoutType(layout) ? layout : 'grid', ...state, }); @@ -74,7 +69,7 @@ export class MetricScene extends SceneObjectBase { } getUrlState() { - return { actionView: this.state.actionView, layout: this.state.layout }; + return { actionView: this.state.actionView }; } updateFromUrl(values: SceneObjectUrlValues) { @@ -88,13 +83,6 @@ export class MetricScene extends SceneObjectBase { } else if (values.actionView === null) { this.setActionView(undefined); } - - if (typeof values.layout === 'string') { - const newLayout = values.layout as LayoutType; - if (this.state.layout !== newLayout) { - this.setState({ layout: newLayout }); - } - } } public setActionView(actionView?: ActionViewType) { diff --git a/public/app/features/trails/interactions.ts b/public/app/features/trails/interactions.ts index 206b0394526..b1903721da8 100644 --- a/public/app/features/trails/interactions.ts +++ b/public/app/features/trails/interactions.ts @@ -1,7 +1,7 @@ import { AdHocVariableFilter } from '@grafana/data'; import { reportInteraction } from '@grafana/runtime'; -import { LayoutType } from './ActionTabs/types'; +import { BreakdownLayoutType } from './ActionTabs/types'; import { TrailStepType } from './DataTrailsHistory'; import { ActionViewType } from './shared'; @@ -26,7 +26,7 @@ type Interactions = { cause: 'breakdown' | 'adhoc_filter'; }; // User changed the breakdown layout - breakdown_layout_changed: { layout: LayoutType }; + breakdown_layout_changed: { layout: BreakdownLayoutType }; // A metric exploration has started due to one of the following causes exploration_started: { cause: (