-
Notifications
You must be signed in to change notification settings - Fork 12
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
Render property display name #137
Conversation
c43b78c
to
42a0b35
Compare
pkg/models/query.go
Outdated
// TwinMakerQuery model | ||
type TwinMakerQuery struct { | ||
GrafanaLiveEnabled bool `json:"grafanaLiveEnabled,omitempty"` | ||
IsStreaming bool `json:"isStreaming,omitempty"` | ||
WorkspaceId string `json:"workspaceId,omitempty"` | ||
EntityId string `json:"entityId,omitempty"` | ||
Properties []*string `json:"properties,omitempty"` | ||
Properties []*TwinmakerPropertyInfo `json:"properties,omitempty"` |
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 change seems like it would break any existing saved dashboards 🤔
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.
nice catch, any suggestion as to how I should handle it.
@@ -263,12 +263,20 @@ export class QueryEditor extends PureComponent<Props, State> { | |||
onPropertiesSelected = (sel: Array<SelectableValue<string>>) => { | |||
const { onChange, query, onRunQuery } = this.props; | |||
let properties: string[] = []; | |||
let propertyDisplayNames: { [key: string]: string } = {}; |
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.
@Digized -- is this the part you expect to do the migration? I think this will only work when in edit mode, but not from a read only saved dashbaord
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.
if a customer has already created a dashboard before updating the plugin:
for all the dashboards created previously, they would still see the propertyName.
when a new dashboard is added, if it has propertyDisplayName, then we use the propertyDisplayName.
just fyi, propertyDisplayName is an optional field.
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.
i get it now :) i added a suggestion for comment in the golang side
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.
looks good
Co-authored-by: Ryan McKinley <ryantxu@gmail.com>
Twinmaker is adding a property displayName field. This pr is to render the property display name in the grafana plugin.