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.

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

Add resource for chart and dashboard #4197

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

AdityaHegde
Copy link
Collaborator

@AdityaHegde AdityaHegde commented Feb 28, 2024

This adds basic resources for chart and dashboard. We are using vega lite config definition for charts. Dashboard will contain a list of charts. Examples,

kind: chart
title: My chart

# Data query
data:
  name: MetricsViewAggregation
  args:
    metrics_view: foo
    dimensions:
      - name: __time
        time_grain: year
    measures:
      - total_sales
    where:
      cond:
        op: GTE
        args:
	        - identifier: bar
	        - value: 'Something'
vega_lite: > 
{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "description": "A simple bar chart with embedded data.",
  "mark": "bar",
  "encoding": {
    "x": {"field": "time", "type": "nominal", "axis": {"labelAngle": 0}},
    "y": {"field": "total_sales", "type": "quantitative"}
  }
}
# dashboards/my_dashboard.yaml
kind: dashboard
title: My dashboard

# Define grid units
grid:
  columns: 12
  rows: 20

# Define components in the grid
components:    
  - chart: my_chart
    columns: 3
    rows: 5
  - chart: chart_2

string title = 1;
string query_name = 2;
string query_args_json = 3;
string vega_lite_config = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Vega calls them "specifications" instead of "configurations", so maybe vega_lite_spec?

Comment on lines 421 to 422
optional int64 columns = 2;
optional int64 rows = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem like these need two null value types – because there's no visual meaning to setting them to 0. So would consider skipping optional and using 0 to indicate "infer".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm not sure if this will work if we want to differentiate between (0,0) and not specified. Honestly lets keep this as is for now. Once we have a prototype from UI side we can tweak this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But lets keep this not optional for now. That makes the most sense and we can change to optional if needed.

Comment on lines +412 to +413
int64 grid_columns = 2;
int64 grid_rows = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

More a discussion than a change request, but – not sure I see the point in a configurable grid, especially not for rows. For rows, wouldn't the size just always depend on the number of components? For columns, it feels like you would want the number to be dynamic depending on the screen width (with each column corresponding to a certain number of pixels, for example).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw this more like total width and height. Take this for example,

<!-- columns=6 rows=5 -->
<div class="grid grid-cols-6 grid-rows-5>
  <!-- chart-1 columns=2 rows=3 -->
  <div class="col-span-2 row-span-3">
  ...
  </div> 
</div>

This is still up in the air TBH. We will probably change this quite a lot once the prototype is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on how we want to layout components but a component will/should be able to occupy varying col and row spans. Imagine a 12x12 grid and you have 1 component occupying 2 columns and 12 rows and then 2 components occupying 10 cols and 6 rows each.


// Query name
if tmp.Data.Name == "" {
return fmt.Errorf(`invalid value %q for property "Data.name"`, tmp.Data.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lowercase Data

@AdityaHegde AdityaHegde force-pushed the adityahegde/custom-dashboard-resources branch from d1dd6bb to bcddc53 Compare February 29, 2024 14:42
@AdityaHegde AdityaHegde force-pushed the adityahegde/custom-dashboard-resources branch from bcddc53 to aabb983 Compare February 29, 2024 14:44
@@ -55,6 +55,7 @@
"date-fns": "^2.30.0",
"jsdom": "^23.0.1",
"litepicker": "^2.0.12",
"lodash.omitby": "^4.6.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was getting a module not found error while running make proto.generate without this.

@begelundmuller begelundmuller merged commit d857d9b into main Mar 1, 2024
7 checks passed
@begelundmuller begelundmuller deleted the adityahegde/custom-dashboard-resources branch March 1, 2024 11:04
begelundmuller added a commit that referenced this pull request Mar 1, 2024
* Update Bookmarks API to support new designs (#4167)

* Update Bookmarks API to support new designs

* Update list bookmarks

* Fix migration sql

* Update to use shared/default for bookmarks

* Fix edit bookmark query

* Update list bookmark

* Update API to use resource kind/name

* Update default resource type

* Set cookies to Lax (#4212)

* Fix Go coverage (#4219)

* Fix Go coverage

* Whoops

* Util for better HTTP error handling (#4220)

* Util for better HTTP error handling

* Self review

* Add resource for chart and dashboard (#4197)

* Add resource for chart and dashboard

* Fix format

* PR comments

* API resource review

---------

Co-authored-by: Aditya Hegde <adityahegderocks@gmail.com>
begelundmuller added a commit that referenced this pull request Mar 1, 2024
* draft commit

* draft commit

* permissions

* review comments - more generic APIs

* review comments - more generic APIs

* review comments - more generic APIs

* remove parsing metric

* revert wrong change

* comment change

* API resource review (#4223)

* Update Bookmarks API to support new designs (#4167)

* Update Bookmarks API to support new designs

* Update list bookmarks

* Fix migration sql

* Update to use shared/default for bookmarks

* Fix edit bookmark query

* Update list bookmark

* Update API to use resource kind/name

* Update default resource type

* Set cookies to Lax (#4212)

* Fix Go coverage (#4219)

* Fix Go coverage

* Whoops

* Util for better HTTP error handling (#4220)

* Util for better HTTP error handling

* Self review

* Add resource for chart and dashboard (#4197)

* Add resource for chart and dashboard

* Fix format

* PR comments

* API resource review

---------

Co-authored-by: Aditya Hegde <adityahegderocks@gmail.com>

---------

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
Co-authored-by: Aditya Hegde <adityahegderocks@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants