-
Notifications
You must be signed in to change notification settings - Fork 384
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
[dashboards] Implement support for sunburst widget #1324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hefty PR with big doc impact! These changes look great, suggested three minor period nits 👍
docs/resources/dashboard.md
Outdated
- **title** (String) The title of the widget. | ||
- **title_align** (String) The alignment of the widget's title. Valid values are `center`, `left`, `right`. | ||
- **title_size** (String) The size of the widget's title (defaults to 16). | ||
- **yaxis** (Block List, Max: 1) A nested block describing the Y-Axis Controls. The structure of this block is described below (see [below for nested schema](#nestedblock--widget--group_definition--widget--timeseries_definition--yaxis)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- **yaxis** (Block List, Max: 1) A nested block describing the Y-Axis Controls. The structure of this block is described below (see [below for nested schema](#nestedblock--widget--group_definition--widget--timeseries_definition--yaxis)) | |
- **yaxis** (Block List, Max: 1) A nested block describing the Y-Axis Controls. The structure of this block is described below. (see [below for nested schema](#nestedblock--widget--group_definition--widget--timeseries_definition--yaxis)) |
docs/resources/dashboard.md
Outdated
@@ -2102,6 +2103,7 @@ Optional: | |||
- **scatterplot_definition** (Block List, Max: 1) The definition for a Scatterplot widget. (see [below for nested schema](#nestedblock--widget--group_definition--widget--scatterplot_definition)) | |||
- **service_level_objective_definition** (Block List, Max: 1) The definition for a Service Level Objective widget. (see [below for nested schema](#nestedblock--widget--group_definition--widget--service_level_objective_definition)) | |||
- **servicemap_definition** (Block List, Max: 1) The definition for a Service Map widget. (see [below for nested schema](#nestedblock--widget--group_definition--widget--servicemap_definition)) | |||
- **sunburst_definition** (Block List, Max: 1) The definition for a Sunburst widget (see [below for nested schema](#nestedblock--widget--group_definition--widget--sunburst_definition)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- **sunburst_definition** (Block List, Max: 1) The definition for a Sunburst widget (see [below for nested schema](#nestedblock--widget--group_definition--widget--sunburst_definition)) | |
- **sunburst_definition** (Block List, Max: 1) The definition for a Sunburst widget. (see [below for nested schema](#nestedblock--widget--group_definition--widget--sunburst_definition)) |
@alai97 thanks for the review! Note that the docs you see here are autogenerated using |
"legend_inline": { | ||
Description: "Used to configure the inline legend. Cannot be used in conjunction with legend_table.", | ||
Type: schema.TypeList, | ||
Optional: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: true, | |
Optional: true, | |
MaxItems: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add conflictsWith property here to make sure both are not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that conflictsWith
will not work here due to the details in this ticket. Am I mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right, completely forgot thanks for linking the PR. What do you think about keeping legend_inline
and legend_table
under one key and document that hide_value
and hide_value
only useful for inline type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion -- I thought about it but decided to split this into two types of legend configurations because we get good enum
validation on legend_inline.type
and legend_table.type
(versus rolling up the legend configurations under one key would also require users to combine the values correctly), so I'm leaning more towards keeping the API as is. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 fair point. We can keep it this way.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
This pull request implements support for the sunburst widget in Terraform.