Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DashboardScene: Fixing major row repeat issues #87539

Merged
merged 15 commits into from
May 14, 2024
Merged

DashboardScene: Fixing major row repeat issues #87539

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
daa9d05
DashboardScene: Fixing major row repeat issues
torkelo May 9, 2024
7c67900
Fixing edit scope
torkelo May 9, 2024
4b258a0
Use dashboard variableDependendency to notify row repeat behaviors
torkelo May 9, 2024
b5e7364
update scenes lib
torkelo May 10, 2024
10bd609
Do not repeat if values are the same
torkelo May 10, 2024
a2fac61
Merge branch 'main' of github.com:grafana/grafana into scene-row-repe…
torkelo May 13, 2024
6e5a743
Update public/app/features/dashboard-scene/scene/DashboardScene.tsx
torkelo May 14, 2024
2e06d79
Updated scenes
torkelo May 14, 2024
9fa50e7
Merge branch 'scene-row-repeat-rethink' of github.com:grafana/grafana…
torkelo May 14, 2024
5564da3
Merge branch 'main' of github.com:grafana/grafana into scene-row-repe…
torkelo May 14, 2024
341df56
Update
torkelo May 14, 2024
bdd13c2
Update
torkelo May 14, 2024
8b0b65a
Do not render row actions for repeated rows
dprokop May 14, 2024
ba76261
Fixed e2e
torkelo May 14, 2024
b252f02
Merge branch 'scene-row-repeat-rethink' of github.com:grafana/grafana…
torkelo May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 9 additions & 8 deletions public/app/features/dashboard-scene/panel-edit/PanelEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,29 +113,30 @@ export class PanelEditor extends SceneObjectBase<PanelEditorState> {
return;
}

if (gridItem instanceof DashboardGridItem) {
this.handleRepeatOptionChanges(gridItem);
} else {
if (!(gridItem instanceof DashboardGridItem)) {
console.error('Unsupported scene object type');
return;
}

this.commitChangesToSource(gridItem);
}

private handleRepeatOptionChanges(panelRepeater: DashboardGridItem) {
let width = panelRepeater.state.width ?? 1;
let height = panelRepeater.state.height;
private commitChangesToSource(gridItem: DashboardGridItem) {
Comment on lines -123 to +124
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unrelated change, and is only here for readability. I have numerous times been very confused trying to find where panel changes are committed (thinking this function only handled repeat options)

let width = gridItem.state.width ?? 1;
let height = gridItem.state.height;

const panelManager = this.state.vizManager;
const horizontalToVertical =
this._initialRepeatOptions.repeatDirection === 'h' && panelManager.state.repeatDirection === 'v';
const verticalToHorizontal =
this._initialRepeatOptions.repeatDirection === 'v' && panelManager.state.repeatDirection === 'h';
if (horizontalToVertical) {
width = Math.floor(width / (panelRepeater.state.maxPerRow ?? 1));
width = Math.floor(width / (gridItem.state.maxPerRow ?? 1));
} else if (verticalToHorizontal) {
width = 24;
}

panelRepeater.setState({
gridItem.setState({
body: panelManager.state.panel.clone(),
repeatDirection: panelManager.state.repeatDirection,
variableName: panelManager.state.repeat,
Expand Down
55 changes: 28 additions & 27 deletions public/app/features/dashboard-scene/scene/RowRepeaterBehavior.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ describe('RowRepeaterBehavior', () => {
// Verify that first row still has repeat behavior
const row1 = grid.state.children[1] as SceneGridRow;
expect(row1.state.$behaviors?.[0]).toBeInstanceOf(RowRepeaterBehavior);
expect(row1.state.$variables!.state.variables[0].getValue()).toBe('1');
expect(row1.state.$variables!.state.variables[0].getValue()).toBe('A1');

const row2 = grid.state.children[2] as SceneGridRow;
expect(row2.state.$variables!.state.variables[0].getValueText?.()).toBe('B');

// Should give repeated panels unique keys
const gridItem = row2.state.children[0] as SceneGridItem;
expect(gridItem.state.body?.state.key).toBe('canvas-1-row-1');
expect(gridItem.state.body?.state.key).toBe('canvas-1-clone-B1');
});

it('Should push row at the bottom down', () => {
Expand Down Expand Up @@ -66,7 +66,7 @@ describe('RowRepeaterBehavior', () => {
it('Should handle second repeat cycle and update remove old repeats', async () => {
// trigger another repeat cycle by changing the variable
const variable = scene.state.$variables!.state.variables[0] as TestVariable;
variable.changeValueTo(['2', '3']);
variable.changeValueTo(['B1', 'C1']);

await new Promise((r) => setTimeout(r, 1));

Expand All @@ -78,12 +78,13 @@ describe('RowRepeaterBehavior', () => {
describe('Given scene empty row', () => {
let scene: EmbeddedScene;
let grid: SceneGridLayout;
let repeatBehavior: RowRepeaterBehavior;
let rowToRepeat: SceneGridRow;

beforeEach(async () => {
({ scene, grid, repeatBehavior } = buildScene({ variableQueryTime: 0 }));
({ scene, grid, rowToRepeat } = buildScene({ variableQueryTime: 0 }));

rowToRepeat.setState({ children: [] });

repeatBehavior.setState({ sources: [] });
activateFullSceneTree(scene);
await new Promise((r) => setTimeout(r, 1));
});
Expand All @@ -108,21 +109,7 @@ interface SceneOptions {
}

function buildScene(options: SceneOptions) {
const repeatBehavior = new RowRepeaterBehavior({
variableName: 'server',
sources: [
new SceneGridItem({
x: 0,
y: 11,
width: 24,
height: 5,
body: new SceneCanvasText({
key: 'canvas-1',
text: 'Panel inside repeated row, server = $server',
}),
}),
],
});
const repeatBehavior = new RowRepeaterBehavior({ variableName: 'server' });

const grid = new SceneGridLayout({
children: [
Expand All @@ -141,6 +128,18 @@ function buildScene(options: SceneOptions) {
width: 24,
height: 1,
$behaviors: [repeatBehavior],
children: [
new SceneGridItem({
x: 0,
y: 11,
width: 24,
height: 5,
body: new SceneCanvasText({
key: 'canvas-1',
text: 'Panel inside repeated row, server = $server',
}),
}),
],
}),
new SceneGridRow({
x: 0,
Expand Down Expand Up @@ -185,17 +184,19 @@ function buildScene(options: SceneOptions) {
includeAll: true,
delayMs: options.variableQueryTime,
optionsToReturn: [
{ label: 'A', value: '1' },
{ label: 'B', value: '2' },
{ label: 'C', value: '3' },
{ label: 'D', value: '4' },
{ label: 'E', value: '5' },
{ label: 'A', value: 'A1' },
{ label: 'B', value: 'B1' },
{ label: 'C', value: 'B1' },
{ label: 'D', value: 'C1' },
{ label: 'E', value: 'D1' },
],
}),
],
}),
body: grid,
});

return { scene, grid, repeatBehavior };
const rowToRepeat = repeatBehavior.parent as SceneGridRow;

return { scene, grid, repeatBehavior, rowToRepeat };
}
67 changes: 41 additions & 26 deletions public/app/features/dashboard-scene/scene/RowRepeaterBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { DashboardRepeatsProcessedEvent } from './types';

interface RowRepeaterBehaviorState extends SceneObjectState {
variableName: string;
sources: SceneGridItemLike[];
}

/**
Expand All @@ -45,6 +44,24 @@ export class RowRepeaterBehavior extends SceneObjectBase<RowRepeaterBehaviorStat
this._performRepeat();
}

private _getRow(): SceneGridRow {
if (!(this.parent instanceof SceneGridRow)) {
throw new Error('RepeatedRowBehavior: Parent is not a SceneGridRow');
}

return this.parent;
}

private _getLayout(): SceneGridLayout {
const layout = sceneGraph.getLayout(this);

if (!(layout instanceof SceneGridLayout)) {
throw new Error('RepeatedRowBehavior: Layout is not a SceneGridLayout');
}

return layout;
}

private _performRepeat() {
if (this._variableDependency.hasDependencyInLoadingState()) {
return;
Expand All @@ -62,40 +79,32 @@ export class RowRepeaterBehavior extends SceneObjectBase<RowRepeaterBehaviorStat
return;
}

if (!(this.parent instanceof SceneGridRow)) {
console.error('RepeatedRowBehavior: Parent is not a SceneGridRow');
return;
}

const layout = sceneGraph.getLayout(this);
const rowToRepeat = this._getRow();
const layout = this._getLayout();

if (!(layout instanceof SceneGridLayout)) {
console.error('RepeatedRowBehavior: Layout is not a SceneGridLayout');
return;
}

const rowToRepeat = this.parent;
const { values, texts } = getMultiVariableValues(variable);
const rows: SceneGridRow[] = [];
const rowContentHeight = getRowContentHeight(this.state.sources);
const rowContent = rowToRepeat.state.children;
const rowContentHeight = getRowContentHeight(rowContent);

let maxYOfRows = 0;

// Loop through variable values and create repeates
for (let index = 0; index < values.length; index++) {
const children: SceneGridItemLike[] = [];
const localValue = values[index];

// Loop through panels inside row
for (const source of this.state.sources) {
for (const source of rowContent) {
const sourceItemY = source.state.y ?? 0;
const itemY = sourceItemY + (rowContentHeight + 1) * index;

const itemClone = source.clone({
key: `${source.state.key}-clone-${index}`,
y: itemY,
});
const itemKey = index > 0 ? `${source.state.key}-clone-${localValue}` : source.state.key;
const itemClone = source.clone({ key: itemKey, y: itemY });

//Make sure all the child scene objects have unique keys
ensureUniqueKeys(itemClone, index);
if (index > 0) {
ensureUniqueKeys(itemClone, localValue);
}

children.push(itemClone);

Expand All @@ -104,7 +113,7 @@ export class RowRepeaterBehavior extends SceneObjectBase<RowRepeaterBehaviorStat
}
}

const rowClone = this.getRowClone(rowToRepeat, index, values[index], texts[index], rowContentHeight, children);
const rowClone = this.getRowClone(rowToRepeat, index, localValue, texts[index], rowContentHeight, children);
rows.push(rowClone);
}

Expand Down Expand Up @@ -136,7 +145,7 @@ export class RowRepeaterBehavior extends SceneObjectBase<RowRepeaterBehaviorStat
const sourceRowY = rowToRepeat.state.y ?? 0;

return rowToRepeat.clone({
key: `${rowToRepeat.state.key}-clone-${index}`,
key: `${rowToRepeat.state.key}-clone-${value}`,
$variables: new SceneVariableSet({
variables: [new LocalValueVariable({ name: this.state.variableName, value, text: String(text) })],
}),
Expand All @@ -145,6 +154,12 @@ export class RowRepeaterBehavior extends SceneObjectBase<RowRepeaterBehaviorStat
y: sourceRowY + rowContentHeight * index + index,
});
}

public removeRepeatClonesFromLayout() {
const layout = this._getLayout();
const children = getLayoutChildrenFilterOutRepeatClones(this._getLayout(), this._getRow());
layout.setState({ children: children });
}
}

function getRowContentHeight(panels: SceneGridItemLike[]): number {
Expand Down Expand Up @@ -207,9 +222,9 @@ function getLayoutChildrenFilterOutRepeatClones(layout: SceneGridLayout, rowToRe
});
}

function ensureUniqueKeys(item: SceneGridItemLike, rowIndex: number) {
function ensureUniqueKeys(item: SceneGridItemLike, localValue: VariableValueSingle) {
item.forEachChild((child) => {
child.setState({ key: `${child.state.key}-row-${rowIndex}` });
ensureUniqueKeys(child, rowIndex);
child.setState({ key: `${child.state.key}-clone-${localValue}` });
ensureUniqueKeys(child, localValue);
});
}
82 changes: 20 additions & 62 deletions public/app/features/dashboard-scene/scene/row-actions/RowActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,7 @@ import { css } from '@emotion/css';
import React from 'react';

import { GrafanaTheme2 } from '@grafana/data';
import {
SceneComponentProps,
SceneGridLayout,
SceneGridRow,
SceneObjectBase,
SceneObjectState,
VizPanel,
} from '@grafana/scenes';
import { SceneComponentProps, SceneGridRow, SceneObjectBase, SceneObjectState, VizPanel } from '@grafana/scenes';
import { Icon, TextLink, useStyles2 } from '@grafana/ui';
import appEvents from 'app/core/app_events';
import { SHARED_DASHBOARD_QUERY } from 'app/plugins/datasource/dashboard';
Expand All @@ -25,31 +18,6 @@ import { RowOptionsButton } from './RowOptionsButton';
export interface RowActionsState extends SceneObjectState {}

export class RowActions extends SceneObjectBase<RowActionsState> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interesting thing i found when testing is that repeated row has row actions available. This means that the repeated row can be repeated further down. It was possible in the previous arch as well, tho it resulted in no effect. I think we should just disable the repeated row's RowActions, otherwise it's confusing and the results are confusing. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, your rigtht, it should not have any options action even

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo - i've just pushed a commit that handles that

private updateLayout(rowClone: SceneGridRow): void {
const row = this.getParent();

const layout = this.getDashboard().state.body;

if (!(layout instanceof SceneGridLayout)) {
throw new Error('Layout is not a SceneGridLayout');
}

// remove the repeated rows
const children = layout.state.children.filter((child) => !child.state.key?.startsWith(`${row.state.key}-clone-`));

// get the index to replace later
const index = children.indexOf(row);

if (index === -1) {
throw new Error('Parent row not found in layout children');
}

// replace the row with the clone
layout.setState({
children: [...children.slice(0, index), rowClone, ...children.slice(index + 1)],
});
}

public getParent(): SceneGridRow {
if (!(this.parent instanceof SceneGridRow)) {
throw new Error('RowActions must have a SceneGridRow parent');
Expand All @@ -64,39 +32,29 @@ export class RowActions extends SceneObjectBase<RowActionsState> {

public onUpdate = (title: string, repeat?: string | null): void => {
const row = this.getParent();
let repeatBehavior: RowRepeaterBehavior | undefined;

// return early if there is no repeat
if (!repeat) {
const clone = row.clone();

// remove the row repeater behaviour, leave the rest
clone.setState({
title,
$behaviors: row.state.$behaviors?.filter((b) => !(b instanceof RowRepeaterBehavior)) ?? [],
});

this.updateLayout(clone);

return;
if (row.state.$behaviors) {
for (let b of row.state.$behaviors) {
if (b instanceof RowRepeaterBehavior) {
repeatBehavior = b;
}
}
}

const children = row.state.children.map((child) => child.clone());

const newBehaviour = new RowRepeaterBehavior({
variableName: repeat,
sources: children,
});

// get rest of behaviors except the old row repeater, if any, and push new one
const behaviors = row.state.$behaviors?.filter((b) => !(b instanceof RowRepeaterBehavior)) ?? [];
behaviors.push(newBehaviour);

row.setState({
title,
$behaviors: behaviors,
});
if (repeat && !repeatBehavior) {
const repeatBehavior = new RowRepeaterBehavior({ variableName: repeat });
row.setState({ $behaviors: [...(row.state.$behaviors ?? []), repeatBehavior] });
// Temp, needs fix in scenes lib
repeatBehavior.activate();
} else if (repeatBehavior) {
repeatBehavior.removeRepeatClonesFromLayout();
row.setState({ $behaviors: row.state.$behaviors!.filter((b) => b !== repeatBehavior) });
}

newBehaviour.activate();
if (title !== row.state.title) {
row.setState({ title });
}
};

public onDelete = () => {
Expand Down