feat(dashboards and charts): editable tiles | slottable menu items #1758

filipgutica · 2024-11-01T05:50:07Z

Summary

https://konghq.atlassian.net/browse/MA-3367

add new edit-tile event to dashboard renderer
forward go-to -explore prop to analytics chart through tile definition
add new editable property to tile definition that will add an "edit" item to the analytics chart kebab menu. Clicking on edit, will emit an @edit-tile event with the entire GridTile object containing the tile layout, definition and index

`@edit-tile` event

{
    "layout": {
        "position": {
            "col": 0,
            "row": 2
        },
        "size": {
            "cols": 3,
            "rows": 2
        }
    },
    "meta": {
        "chart": {
            "type": "horizontal_bar",
            "chartTitle": "Horizontal bar chart of mock data",
            "allowCsvExport": true
        },
        "query": {
            "datasource": "basic",
            "dimensions": [
                "route"
            ]
        },
        "editable": true
    },
    "id": 3
}

adorack

I'd like to review.

filipgutica · 2024-11-01T16:17:16Z

packages/analytics/dashboard-renderer/src/components/GoldenSignalsRenderer.vue

@@ -46,18 +46,24 @@ const overrideTimeframe: Ref<Timeframe> = computed(() => {
  return relativePeriod
 })

-const options = computed<ProviderProps>(() => ({


There was a type error here that's somehow getting past CI... 🤔

No overload matches this call. Overload 1 of 2, '(getter: ComputedGetter<Partial<{ description: string; containerTitle: string; refreshInterval: number; datasource: "advanced" | "basic"; dimension: "api_product" | "api_product_version" | "control_plane" | ... 9 more ... | "iso_code"; ... 8 more ...; percentileLatency: boolean; }> & Omit<...>>, debugOptions?: DebuggerOptions | undefined): ComputedRef<...>', gave the following error. Type '"advanced" | "basic" | "ai"' is not assignable to type '"advanced" | "basic" | undefined'. Type '"ai"' is not assignable to type '"advanced" | "basic" | undefined'. Overload 2 of 2, '(options: WritableComputedOptions<Partial<{ description: string; containerTitle: string; refreshInterval: number; datasource: "advanced" | "basic"; dimension: "api_product" | "api_product_version" | "control_plane" | ... 9 more ... | "iso_code"; ... 8 more ...; percentileLatency: boolean; }> & Omit<...>, Partial<...> & Omit<...>>, debugOptions?: DebuggerOptions | undefined): WritableComputedRef<...>', gave the following error. Argument of type '() => { datasource: "advanced" | "basic" | "ai"; overrideTimeframe: Timeframe; tz: string; additionalFilter: ExploreFilter[]; longCardTitles: boolean | undefined; containerTitle: string | undefined; description: string | undefined; percentileLatency: boolean | undefined; refreshInterval: number; queryReady: boolean; }' is not assignable to parameter of type 'WritableComputedOptions<Partial<{ description: string; containerTitle: string; refreshInterval: number; datasource: "advanced" | "basic"; dimension: "api_product" | "api_product_version" | "control_plane" | ... 9 more ... | "iso_code"; ... 8 more ...; percentileLatency: boolean; }> & Omit<...>, Partial<...> & Omit<....'.ts-plugin(2769) MetricsProvider.vue.d.ts(67, 5): The expected type comes from property 'datasource' which is declared here on type 'Partial<{ description: string; containerTitle: string; refreshInterval: number; datasource: "advanced" | "basic"; dimension: "api_product" | "api_product_version" | "control_plane" | ... 9 more ... | "iso_code"; ... 8 more ...; percentileLatency: boolean; }> & Omit<...>'

I think it's from updates to analytics-utilities which introduced the ai datasource, while the provider props in metrics provider defines datasource property like so: datasource?: "basic" | "advanced" | undefined;

Not sure if my change is the right one, or we should add ai to the acceptable datasources in MetricsProvider

Hmmm. I'll make a note to look at this later. I think this is the fault of the schema; it shouldn't allow setting ai as a datasource for metric cards. I think what you have for now is fine.

- add new edit-tile event to dashboard renderer - forward go to explore to analytics chart through tile definition - add new editable property to tile definition that will add an "edit" item to the analytics chart kebab menu. Clicking on edit, will emit an @edit-tile event with the entire GridTile<T> object containing the tile layout, definition and index

adorack · 2024-11-01T17:07:24Z

packages/analytics/dashboard-renderer/src/types/dashboard-renderer-types.ts

@@ -55,6 +55,14 @@ const allowCsvExport = {
  type: 'boolean',
 } as const

+const goToExplore = {


I don't think either of these properties should be exposed in the schema. The things in the schema are eventually going to be part of the public API for dashboards; there's no case where an end user should be able to directly configure these props.

I'd recommend:

editable is exposed via context. I think it's OK (especially for now) to say that either all tiles are editable or none are.

goToExplore is probably a base URL that's configured via the analytics bridge, and the query data provider calculates the final URL to hand to AnalyticsChart.

How does the dashboard know if the tiles should have go to explore or not? 🤔

I'm assuming some tiles will support it while some would not. EG TopNTable doesn't map to anything in Explore, same with "slottable" tiles

adorack · 2024-11-01T17:13:01Z

packages/analytics/dashboard-renderer/src/components/GoldenSignalsRenderer.vue

@@ -46,18 +46,24 @@ const overrideTimeframe: Ref<Timeframe> = computed(() => {
  return relativePeriod
 })

-const options = computed<ProviderProps>(() => ({


Hmmm. I'll make a note to look at this later. I think this is the fault of the schema; it shouldn't allow setting ai as a datasource for metric cards. I think what you have for now is fine.

adorack · 2024-11-01T21:20:06Z

packages/analytics/dashboard-renderer/src/components/BarChartRenderer.vue

@@ -3,7 +3,7 @@
  <BaseAnalyticsChartRenderer
    :chart-options="chartOptions"
    :context="context"
-    :editable="editable"
+    :editable="context.editable"


You're already passing in context -- no need for the extra prop. :)

adorack · 2024-11-01T21:20:57Z

packages/analytics/dashboard-renderer/src/components/DashboardTile.vue

@@ -61,7 +61,7 @@ const componentData = computed(() => {
      queryReady: props.queryReady,
      chartOptions: props.definition.chart,
      height: props.height - PADDING_SIZE * 2,
-      editable: props.definition.editable,
+      editable: props.context.editable,


I don't think you need to pass in this prop.

It looks like it's still there...

Ahh, my bad. I got rid of using the prop, forgot to revert this line 😅
24443a5

adorack · 2024-11-01T21:21:29Z

packages/analytics/dashboard-renderer/src/types/dashboard-renderer-types.ts

@@ -59,10 +60,6 @@ const goToExplore = {
  type: 'string',
 } as const


Please remove goToExplore as well.

adorack · 2024-11-04T16:00:00Z

packages/analytics/dashboard-renderer/src/components/DashboardTile.vue

@@ -61,7 +61,7 @@ const componentData = computed(() => {
      queryReady: props.queryReady,
      chartOptions: props.definition.chart,
      height: props.height - PADDING_SIZE * 2,
-      editable: props.definition.editable,
+      editable: props.context.editable,


It looks like it's still there...

filipgutica requested review from adorack and mihai-peteu November 1, 2024 05:50

filipgutica requested a review from a team as a code owner November 1, 2024 05:50

filipgutica force-pushed the feat/dashboard-renderer-go-to-explore branch from ee386d3 to e20f568 Compare November 1, 2024 05:54

adorack requested changes Nov 1, 2024

View reviewed changes

filipgutica force-pushed the feat/dashboard-renderer-go-to-explore branch from e20f568 to 39d5b82 Compare November 1, 2024 16:16

filipgutica commented Nov 1, 2024

View reviewed changes

filipgutica force-pushed the feat/dashboard-renderer-go-to-explore branch from 39d5b82 to b103fc2 Compare November 1, 2024 16:59

adorack requested changes Nov 1, 2024

View reviewed changes

fix: move editable to context

231a753

filipgutica force-pushed the feat/dashboard-renderer-go-to-explore branch from bd46c63 to 231a753 Compare November 1, 2024 20:00

filipgutica requested a review from adorack November 1, 2024 21:18

adorack requested changes Nov 1, 2024

View reviewed changes

fix: address comment

24443a5

adorack requested changes Nov 4, 2024

View reviewed changes

fix: address comment

fd442ec

filipgutica requested a review from adorack November 4, 2024 18:30

filipgutica enabled auto-merge (squash) November 4, 2024 18:35

adorack approved these changes Nov 4, 2024

View reviewed changes

filipgutica merged commit 2b0a8fd into main Nov 4, 2024
10 checks passed

filipgutica deleted the feat/dashboard-renderer-go-to-explore branch November 4, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboards and charts): editable tiles | slottable menu items #1758

feat(dashboards and charts): editable tiles | slottable menu items #1758

filipgutica commented Nov 1, 2024 •

edited

Loading

adorack left a comment

filipgutica Nov 1, 2024

filipgutica Nov 1, 2024

adorack Nov 1, 2024

adorack Nov 1, 2024 •

edited

Loading

filipgutica Nov 1, 2024

filipgutica Nov 1, 2024

adorack Nov 1, 2024

adorack Nov 1, 2024

filipgutica Nov 1, 2024

adorack Nov 1, 2024

filipgutica Nov 1, 2024

adorack Nov 4, 2024

filipgutica Nov 4, 2024

adorack Nov 1, 2024

filipgutica Nov 1, 2024

adorack Nov 4, 2024

		@@ -59,10 +60,6 @@ const goToExplore = {
		type: 'string',
		} as const

feat(dashboards and charts): editable tiles | slottable menu items #1758

feat(dashboards and charts): editable tiles | slottable menu items #1758

Conversation

filipgutica commented Nov 1, 2024 • edited Loading

Summary

@edit-tile event

adorack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adorack Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipgutica commented Nov 1, 2024 •

edited

Loading

`@edit-tile` event

adorack Nov 1, 2024 •

edited

Loading